This is the mail archive of the cygwin-patches mailing list for the Cygwin project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Hi Joe, thanks for the patch. Review inline. On Jun 9 15:44, Joe Lowe wrote: > 2nd pass at reparse point handling patch. > [...] > static inline int > readdir_check_reparse_point (POBJECT_ATTRIBUTES attr) > { > - DWORD ret = DT_UNKNOWN; > + int ret = DT_UNKNOWN; Given that d_type is an unsigned char type, maybe it would be better to change the return type of the function (and `ret') accordingly instead. > IO_STATUS_BLOCK io; > HANDLE reph; > UNICODE_STRING subst; > @@ -197,20 +195,11 @@ readdir_check_reparse_point (POBJECT_ATTRIBUTES attr) > &io, FSCTL_GET_REPARSE_POINT, NULL, 0, > (LPVOID) rp, MAXIMUM_REPARSE_DATA_BUFFER_SIZE))) > { > - if (rp->ReparseTag == IO_REPARSE_TAG_MOUNT_POINT) > - { > - RtlInitCountedUnicodeString (&subst, > - (WCHAR *)((char *)rp->MountPointReparseBuffer.PathBuffer > - + rp->MountPointReparseBuffer.SubstituteNameOffset), > - rp->MountPointReparseBuffer.SubstituteNameLength); > - /* Only volume mountpoints are treated as directories. */ > - if (RtlEqualUnicodePathPrefix (&subst, &ro_u_volume, TRUE)) > - ret = DT_DIR; > - else > - ret = DT_LNK; > - } > - else if (rp->ReparseTag == IO_REPARSE_TAG_SYMLINK) > - ret = DT_LNK; > + /* Need to agree with path_conv, so use common helper logic. > + */ > + ret = get_reparse_point_type (rp, false/*remote*/, &subst); > + if (ret == DT_UNKNOWN) > + ret = DT_DIR; This looks not right to me. Every unknown type is a dir? Unknown is unknown and should stay this way, that's why the DT_UNKNOWN has been defined as valid type. > @@ -2027,13 +2016,23 @@ fhandler_disk_file::readdir_helper (DIR *dir, dirent *de, DWORD w32_err, > > InitializeObjectAttributes (&attr, fname, pc.objcaseinsensitive (), > get_handle (), NULL); > - de->d_type = readdir_check_reparse_point (&attr); > - if (de->d_type == DT_DIR) > + switch (readdir_check_reparse_point (&attr)) > { > - /* Volume mountpoints are treated as directories. We have to fix > - the inode number, otherwise we have the inode number of the > - mount point, rather than the inode number of the toplevel > - directory of the mounted drive. */ > + case DT_LNK: > + de->d_type = DT_LNK; > + break; > + case DT_UNKNOWN: > + /* Unknown reparse point type: HSM, dedup, compression, ... > + Treat as normal directory. */ > + de->d_type = DT_DIR; Same here. > diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc > index 7d1d23d72..cd62355d7 100644 > --- a/winsup/cygwin/path.cc > +++ b/winsup/cygwin/path.cc > @@ -2261,6 +2261,86 @@ symlink_info::check_sysfile (HANDLE h) > return res; > } > > +static bool > +check_reparse_point_target (PUNICODE_STRING subst) > +{ > + /* Native junction reparse points, or native non-relative > + symbolic links, can be treated as posix symlinks only > + if the SubstituteName can be converted from a native NT > + object namespace name to a win32 name. We only know how > + to convert names with two prefixes : > + "\??\UNC\..." > + "\??\X:..." > + Other reparse points will be treated as files or > + directories, not as posix symlinks. Possible values > + include: > + "\??\Volume{..." > + "\Device\HarddiskVolume1\..." > + "\Device\Lanman\...\..." > + */ > + if (RtlEqualUnicodePathPrefix( subst, &ro_u_uncp, TRUE)) > + { > + return true; > + } Please drop the braces for single line branches. > + else if (RtlEqualUnicodePathPrefix (subst, &ro_u_natp, TRUE) && The `else' is gratuitous, the if branch returns anyway. The case sensitivity parameter can be FALSE here. > + subst->Length >= 6*sizeof(WCHAR) && subst->Buffer[5] == ':' && > + (subst->Length == 6*sizeof(WCHAR) || subst->Buffer[6] == '\\')) wchar literals should have a leading L, as in L':'. > + { > + return true; > + } > + return false; Also, wouldn't it make sense to reorder the code a bit, to avoid the uncp comparison for local paths, given that most access is local anyway? Kind of like if (RtlEqualUnicodePathPrefix (subst, &ro_u_natp, TRUE)) { if (':' && ('\0' || '\\')) return true; if (uncp) return true; } return false; I think it would be ok to just check for "UNC\\" then, as in wcsncmp (subst->Buffer + 4, L"UNC\\", 4) > +} > + > +int > +get_reparse_point_type (PREPARSE_DATA_BUFFER rp, bool remote, PUNICODE_STRING subst) Same as for readdir_check_reparse_point, I guess this should return unsigned char. > +{ > + if (rp->ReparseTag == IO_REPARSE_TAG_SYMLINK) > + { > + /* Windows evaluates native symlink literally. If a remote symlink > + points to, say, C:\foo, it will be handled as if the target is the > + local file C:\foo. That comes in handy since that's how symlinks > + are treated under POSIX as well. */ > + RtlInitCountedUnicodeString (subst, > + (WCHAR *)((char *)rp->SymbolicLinkReparseBuffer.PathBuffer > + + rp->SymbolicLinkReparseBuffer.SubstituteNameOffset), > + rp->SymbolicLinkReparseBuffer.SubstituteNameLength); > + /* Native symlinks are treated as posix symlinks if relative or if the > + target has a prefix that we know how to deal with. > + */ > + if ((rp->SymbolicLinkReparseBuffer.Flags & SYMLINK_FLAG_RELATIVE) || > + check_reparse_point_target (subst)) > + return DT_LNK; > + } > + else if (rp->ReparseTag == IO_REPARSE_TAG_MOUNT_POINT) > + { > + RtlInitCountedUnicodeString (subst, > + (WCHAR *)((char *)rp->MountPointReparseBuffer.PathBuffer > + + rp->MountPointReparseBuffer.SubstituteNameOffset), > + rp->MountPointReparseBuffer.SubstituteNameLength); > + /* Don't handle junctions on remote filesystems as symlinks. This type > + of reparse point is handled transparently by the OS so that the > + target of the junction is the remote directory it is supposed to > + point to. If we handle it as symlink, it will be mistreated as > + pointing to a dir on the local system. */ > + if (remote) > + return DT_DIR; > + /* Native mount points are treated as posix symlinks only if > + the target has a prefix that does not indicate a volume > + mount point and that we know how to deal with. > + */ > + if (check_reparse_point_target (subst)) > + return DT_LNK; > + } > + else > + { > + /* Maybe it's a reparse point, but it's certainly not one we recognize. > + */ > + memset (subst, 0, sizeof (*subst)); > + return DT_UNKNOWN; > + } > + return DT_DIR; Same thing with `else' here. Especially the last else branch is confusing, given that it always leads to a return from the function so this last line is unreachable code (which Coverity will complain about). The unassuming reader might wonder why the function defaults to DT_DIR while in fact it doesn't. > @@ -2944,22 +3017,25 @@ restart: > &= ~FILE_ATTRIBUTE_DIRECTORY; > break; > } > - else > + else if (res == -1) > { > - /* Volume moint point or unrecognized reparse point type. > + /* Volume moint point or unhandled reparse point. > Make sure the open handle is not used in later stat calls. > The handle has been opened with the FILE_OPEN_REPARSE_POINT > flag, so it's a handle to the reparse point, not a handle > - to the volumes root dir. */ > + to the reparse point target. */ > pflags &= ~PC_KEEP_HANDLE; > - /* Volume mount point: The filesystem information for the top > - level directory should be for the volume top level directory, > - rather than for the reparse point itself. So we fetch the > - filesystem information again, but with a NULL handle. > - This does what we want because fs_info::update opens the > - handle without FILE_OPEN_REPARSE_POINT. */ > - if (res == -1) > - fs.update (&upath, NULL); > + /* The filesystem information should be for the target of > + the reparse point rather than for the reparse point itself. > + So we fetch the filesystem information again, but with a > + NULL handle. This does what we want because fs_info::update > + opens the handle without FILE_OPEN_REPARSE_POINT. */ > + fs.update (&upath, NULL); > + } > + else > + { > + /* Unknown reparse point type: HSM, dedup, compression, ... > + Treat as normal directory. */ > } Nothing against reordering the code, but this drops removing PC_KEEP_HANDLE from pflags if res == 0, i.e., for unknown reparse points. Thanks, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat
Attachment:
signature.asc
Description: PGP signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |