From c36cd56c548a27683e64b93348bbe9ad1d47b1ea Mon Sep 17 00:00:00 2001 From: Corinna Vinschen Date: Fri, 17 Jun 2011 11:04:44 +0000 Subject: [PATCH] * 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. --- winsup/cygwin/ChangeLog | 24 ++++++ winsup/cygwin/fhandler.cc | 14 ++-- winsup/cygwin/fhandler.h | 17 +++- winsup/cygwin/fhandler_disk_file.cc | 121 +++++++++++++++++++++++++--- 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,3 +1,27 @@ +2011-06-17 Corinna Vinschen + + * 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 * dcrt0.cc (dll_crt0_1): Call strace.dll_info after call to pinfo_init. 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); } @@ -1372,6 +1372,35 @@ fhandler_disk_file::open (int flags, mode_t mode) return open_fs (flags, 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) { @@ -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);