* fhandler.cc (fhandler_base::open): Drop local create_options variable.

	Use options member instead.
	* fhandler.h (class fhandler_base): Change type of access member to
	ACCESS_MASK.  Change get_access and set_access methods accordingly.
	Add options member.  Add get_options and set_options methods.
	(class fhandler_disk_file): Add prw_handle.
	(fhandler_disk_file::prw_open): Declare.
	(fhandler_disk_file::close): Declare.
	(fhandler_disk_file::dup): Declare.
	(fhandler_disk_file::fixup_after_fork): Declare.
	* fhandler_disk_file.cc (fhandler_disk_file::fhandler_disk_file):
	Initialize prw_handle to NULL.
	(fhandler_disk_file::close): Close prw_handle.
	(fhandler_disk_file::dup): New method.
	(fhandler_disk_file::fixup_after_fork): Set prw_handle to NULL since
	prw_handle is not inherited.
	(fhandler_disk_file::prw_open): New method.  Add long comment to
	explain current behaviour.
	(fhandler_disk_file::pread): Revert previous change.  Change to use
	prw_handle if possible.
	(fhandler_disk_file::pwrite): Change to use prw_handle if possible.

4 files changed, 155 insertions, 21 deletions

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index 3b9b57bde..9ee89e9b2 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,5 +1,29 @@
 2011-06-17  Corinna Vinschen  <corinna@vinschen.de>
 
+	* fhandler.cc (fhandler_base::open): Drop local create_options variable.
+	Use options member instead.
+	* fhandler.h (class fhandler_base): Change type of access member to
+	ACCESS_MASK.  Change get_access and set_access methods accordingly.
+	Add options member.  Add get_options and set_options methods.
+	(class fhandler_disk_file): Add prw_handle.
+	(fhandler_disk_file::prw_open): Declare.
+	(fhandler_disk_file::close): Declare.
+	(fhandler_disk_file::dup): Declare.
+	(fhandler_disk_file::fixup_after_fork): Declare.
+	* fhandler_disk_file.cc (fhandler_disk_file::fhandler_disk_file):
+	Initialize prw_handle to NULL.
+	(fhandler_disk_file::close): Close prw_handle.
+	(fhandler_disk_file::dup): New method.
+	(fhandler_disk_file::fixup_after_fork): Set prw_handle to NULL since
+	prw_handle is not inherited.
+	(fhandler_disk_file::prw_open): New method.  Add long comment to
+	explain current behaviour.
+	(fhandler_disk_file::pread): Revert previous change.  Change to use
+	prw_handle if possible.
+	(fhandler_disk_file::pwrite): Change to use prw_handle if possible.
+
+2011-06-17  Corinna Vinschen  <corinna@vinschen.de>
+
 	* dcrt0.cc (dll_crt0_1): Call strace.dll_info after call to pinfo_init.
 	* strace.cc (strace::hello): Drop printing DLL information here since
 	application info is not always available at this point.
diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
index 4ea84974e..7e01f672b 100644
--- a/winsup/cygwin/fhandler.cc
+++ b/winsup/cygwin/fhandler.cc
@@ -492,7 +492,6 @@ fhandler_base::open (int flags, mode_t mode)
   ULONG file_attributes = 0;
   ULONG shared = (get_major () == DEV_TAPE_MAJOR ? 0 : FILE_SHARE_VALID_FLAGS);
   ULONG create_disposition;
-  ULONG create_options = FILE_OPEN_FOR_BACKUP_INTENT;
   OBJECT_ATTRIBUTES attr;
   IO_STATUS_BLOCK io;
   NTSTATUS status;
@@ -503,6 +502,7 @@ fhandler_base::open (int flags, mode_t mode)
 
   pc.get_object_attr (attr, *sec_none_cloexec (flags));
 
+  options = FILE_OPEN_FOR_BACKUP_INTENT;
   switch (query_open ())
     {
       case query_read_control:
@@ -528,12 +528,12 @@ fhandler_base::open (int flags, mode_t mode)
 	else
 	  access = GENERIC_READ | GENERIC_WRITE;
 	if (flags & O_SYNC)
-	  create_options |= FILE_WRITE_THROUGH;
+	  options |= FILE_WRITE_THROUGH;
 	if (flags & O_DIRECT)
-	  create_options |= FILE_NO_INTERMEDIATE_BUFFERING;
+	  options |= FILE_NO_INTERMEDIATE_BUFFERING;
 	if (get_major () != DEV_SERIAL_MAJOR && get_major () != DEV_TAPE_MAJOR)
 	  {
-	    create_options |= FILE_SYNCHRONOUS_IO_NONALERT;
+	    options |= FILE_SYNCHRONOUS_IO_NONALERT;
 	    access |= SYNCHRONIZE;
 	  }
 	break;
@@ -574,7 +574,7 @@ fhandler_base::open (int flags, mode_t mode)
       /* Add the reparse point flag to native symlinks, otherwise we open the
 	 target, not the symlink.  This would break lstat. */
       if (pc.is_rep_symlink ())
-	create_options |= FILE_OPEN_REPARSE_POINT;
+	options |= FILE_OPEN_REPARSE_POINT;
 
       /* Starting with Windows 2000, when trying to overwrite an already
 	 existing file with FILE_ATTRIBUTE_HIDDEN and/or FILE_ATTRIBUTE_SYSTEM
@@ -626,7 +626,7 @@ fhandler_base::open (int flags, mode_t mode)
     }
 
   status = NtCreateFile (&fh, access, &attr, &io, NULL, file_attributes, shared,
-			 create_disposition, create_options, p, plen);
+			 create_disposition, options, p, plen);
   if (!NT_SUCCESS (status))
     {
       /* Trying to create a directory should return EISDIR, not ENOENT. */
@@ -672,7 +672,7 @@ done:
   debug_printf ("%x = NtCreateFile "
 		"(%p, %x, %S, io, NULL, %x, %x, %x, %x, NULL, 0)",
 		status, fh, access, pc.get_nt_native_path (), file_attributes,
-		shared, create_disposition, create_options);
+		shared, create_disposition, options);
 
   syscall_printf ("%d = fhandler_base::open (%S, %p)",
 		  res, pc.get_nt_native_path (), flags);
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 8268a9ae7..5120a3ce2 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -152,7 +152,9 @@ class fhandler_base
   } status, open_status;
 
  private:
-  int access;
+  ACCESS_MASK access;
+  ULONG options;
+
   HANDLE io_handle;
 
   __ino64_t ino;	/* file ID or hashed filename, depends on FS. */
@@ -206,8 +208,11 @@ class fhandler_base
   DWORD get_minor () { return dev ().get_minor (); }
   virtual int get_unit () { return dev ().get_minor (); }
 
-  int get_access () const { return access; }
-  void set_access (int x) { access = x; }
+  ACCESS_MASK get_access () const { return access; }
+  void set_access (ACCESS_MASK x) { access = x; }
+
+  ULONG get_options () const { return options; }
+  void set_options (ULONG x) { options = x; }
 
   int get_flags () { return openflags; }
   void set_flags (int x, int supplied_bin = 0);
@@ -801,13 +806,19 @@ class fhandler_dev_tape: public fhandler_dev_raw
 
 class fhandler_disk_file: public fhandler_base
 {
+  HANDLE prw_handle;
   int readdir_helper (DIR *, dirent *, DWORD, DWORD, PUNICODE_STRING fname) __attribute__ ((regparm (3)));
 
+  int prw_open (bool);
+
  public:
   fhandler_disk_file ();
   fhandler_disk_file (path_conv &pc);
 
   int open (int flags, mode_t mode);
+  int close ();
+  int dup (fhandler_base *child);
+  void fixup_after_fork (HANDLE parent);
   int lock (int, struct __flock64 *);
   bool isdevice () const { return false; }
   int __stdcall fstat (struct __stat64 *buf) __attribute__ ((regparm (2)));
diff --git a/winsup/cygwin/fhandler_disk_file.cc b/winsup/cygwin/fhandler_disk_file.cc
index a57427b4f..9cebbe475 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -1356,12 +1356,12 @@ fhandler_base::utimens_fs (const struct timespec *tvp)
 }
 
 fhandler_disk_file::fhandler_disk_file () :
-  fhandler_base ()
+  fhandler_base (), prw_handle (NULL)
 {
 }
 
 fhandler_disk_file::fhandler_disk_file (path_conv &pc) :
-  fhandler_base ()
+  fhandler_base (), prw_handle (NULL)
 {
   set_name (pc);
 }
@@ -1373,6 +1373,35 @@ fhandler_disk_file::open (int flags, mode_t mode)
 }
 
 int
+fhandler_disk_file::close ()
+{
+  if (prw_handle)
+    NtClose (prw_handle);
+  return fhandler_base::close ();
+}
+
+int
+fhandler_disk_file::dup (fhandler_base *child)
+{
+  fhandler_disk_file *fhc = (fhandler_disk_file *) child;
+
+  int ret = fhandler_base::dup (child);
+  if (!ret && prw_handle
+      && !DuplicateHandle (GetCurrentProcess (), prw_handle,
+			   GetCurrentProcess (), &fhc->prw_handle,
+			   0, TRUE, DUPLICATE_SAME_ACCESS))
+    prw_handle = NULL;
+  return ret;
+}
+
+void
+fhandler_disk_file::fixup_after_fork (HANDLE parent)
+{
+  prw_handle = NULL;
+  fhandler_base::fixup_after_fork (parent);
+}
+
+int
 fhandler_base::open_fs (int flags, mode_t mode)
 {
   /* Unfortunately NT allows to open directories for writing, but that's
@@ -1412,6 +1441,73 @@ out:
   return res;
 }
 
+/* POSIX demands that pread/pwrite don't change the current file position.
+   While NtReadFile/NtWriteFile support atomic seek-and-io, both change the
+   file pointer if the file handle has been opened for synchonous I/O.
+   Using this handle for pread/pwrite would break atomicity, because the
+   read/write operation would have to be followed by a seek back to the old
+   file position.  What we do is to open another handle to the file on the
+   first call to either pread or pwrite.  This is used for any subsequent
+   pread/pwrite.  Thus the current file position of the "normal" file
+   handle is not touched.
+   
+   FIXME:
+
+   Note that this is just a hack.  The problem with this approach is that
+   a change to the file permissions might disallow to open the file with
+   the required permissions to read or write.  This appears to be a border case,
+   but that's exactly what git does.  It creates the file for reading and
+   writing and after writing it, it chmods the file to read-only.  Then it
+   calls pread on the file to examine the content.  This works, but if git
+   would use the original handle to pwrite to the file, it would be broken
+   with our approach.
+
+   One way to implement this is to open the pread/pwrite handle right at
+   file open time.  We would simply maintain two handles, which wouldn't
+   be much of a problem given how we do that for other fhandler types as
+   well.
+
+   However, ultimately fhandler_disk_file should become a derived class of
+   fhandler_base_overlapped.  Each raw_read or raw_write would fetch the
+   actual file position, read/write from there, and then set the file
+   position again.  Fortunately, while the file position is not maintained
+   by the I/O manager, it can be fetched and set to a new value by all
+   processes holding a handle to that file object.  Pread and pwrite differ
+   from raw_read and raw_write just by not touching the current file pos.
+   Actually they could be merged with raw_read/raw_write if we add a position
+   parameter to the latter. */
+
+int
+fhandler_disk_file::prw_open (bool write)
+{
+  NTSTATUS status;
+  IO_STATUS_BLOCK io;
+  OBJECT_ATTRIBUTES attr;
+
+  /* First try to open with the original access mask */
+  ACCESS_MASK access = get_access ();
+  pc.init_reopen_attr (&attr, get_handle ());
+  status = NtOpenFile (&prw_handle, access, &attr, &io,
+		       FILE_SHARE_VALID_FLAGS, get_options ());
+  if (status == STATUS_ACCESS_DENIED)
+    {
+      /* If we get an access denied, chmod has been called.  Try again
+	 with just the required rights to perform the called function. */
+      access &= write ? ~GENERIC_READ : ~GENERIC_WRITE;
+      status = NtOpenFile (&prw_handle, access, &attr, &io,
+			   FILE_SHARE_VALID_FLAGS, get_options ());
+    }
+  debug_printf ("%x = NtOpenFile (%p, %x, %S, io, %x, %x)",
+		status, prw_handle, access, pc.get_nt_native_path (),
+		FILE_SHARE_VALID_FLAGS, get_options ());
+  if (!NT_SUCCESS (status))
+    {
+      __seterrno_from_nt_status (status);
+      return -1;
+    }
+  return 0;
+}
+
 ssize_t __stdcall
 fhandler_disk_file::pread (void *buf, size_t count, _off64_t offset)
 {
@@ -1429,7 +1525,9 @@ fhandler_disk_file::pread (void *buf, size_t count, _off64_t offset)
       IO_STATUS_BLOCK io;
       LARGE_INTEGER off = { QuadPart:offset };
 
-      status = NtReadFile (get_handle (), NULL, NULL, NULL, &io, buf, count,
+      if (!prw_handle && prw_open (false))
+	goto non_atomic;
+      status = NtReadFile (prw_handle, NULL, NULL, NULL, &io, buf, count,
 			   &off, NULL);
       if (!NT_SUCCESS (status))
 	{
@@ -1440,15 +1538,15 @@ fhandler_disk_file::pread (void *buf, size_t count, _off64_t offset)
 	    }
 	  if (status == (NTSTATUS) STATUS_ACCESS_VIOLATION)
 	    {
-	      if (is_at_eof (get_handle ()))
+	      if (is_at_eof (prw_handle))
 		return 0;
 	      switch (mmap_is_attached_or_noreserve (buf, count))
 		{
 		case MMAP_NORESERVE_COMMITED:
-		  status = NtReadFile (get_handle (), NULL, NULL, NULL, &io,
+		  status = NtReadFile (prw_handle, NULL, NULL, NULL, &io,
 				       buf, count, &off, NULL);
 		  if (NT_SUCCESS (status))
-		    goto success;
+		    return io.Information;
 		  break;
 		case MMAP_RAISE_SIGBUS:
 		  raise (SIGBUS);
@@ -1459,12 +1557,9 @@ fhandler_disk_file::pread (void *buf, size_t count, _off64_t offset)
 	  __seterrno_from_nt_status (status);
 	  return -1;
 	}
-
-success:
-      lseek (-io.Information, SEEK_CUR);
-      return io.Information;
     }
 
+non_atomic:
   /* Text mode stays slow and non-atomic. */
   ssize_t res;
   _off64_t curpos = lseek (0, SEEK_CUR);
@@ -1499,7 +1594,9 @@ fhandler_disk_file::pwrite (void *buf, size_t count, _off64_t offset)
       IO_STATUS_BLOCK io;
       LARGE_INTEGER off = { QuadPart:offset };
 
-      status = NtWriteFile (get_handle (), NULL, NULL, NULL, &io, buf, count,
+      if (!prw_handle && prw_open (true))
+	goto non_atomic;
+      status = NtWriteFile (prw_handle, NULL, NULL, NULL, &io, buf, count,
 			    &off, NULL);
       if (!NT_SUCCESS (status))
 	{
@@ -1508,6 +1605,8 @@ fhandler_disk_file::pwrite (void *buf, size_t count, _off64_t offset)
 	}
       return io.Information;
     }
+
+non_atomic:
   /* Text mode stays slow and non-atomic. */
   int res;
   _off64_t curpos = lseek (0, SEEK_CUR);