Re: access () and path.cc
At 01:10 AM 2/28/2003 -0500, Christopher Faylor wrote: >On Fri, Feb 28, 2003 at 01:02:58AM -0500, Pierre A. Humblet wrote: >>At 12:56 AM 2/28/2003 -0500, you wrote: >>>On Fri, Feb 28, 2003 at 12:49:59AM -0500, Pierre A. Humblet wrote: OK, following Chris' remarks here is a much smaller set of changes. >>> >>>Do you think it would make sense to do something along the lines >>>of: + path_conv pc (cfd->is_device ? cfd->get_name () : >>cfd->get_win32_name (), PC_SYM_NOFOLLOW); >> >>I guess one could but judging from the times I see in >>strace it's not really justified. >> >>On the other hand that's something that we could look at after you >>integrate your code. There could eventually be a single get_name >>returning what's appropriate. > >This isn't an issue with my code, at least for fstat64. That's one of >the reasons for my changes. I was trying to minimize the number of >duplicate passes through path_conv::check. I still have some tweaking >to do though since adding path_conv to fhandler balloons the sizes of >fhandler_base and ends up using a lot more space in cygheap, which means >more memory to copy on fork, which could mean slower forks, which means >that my performance improvement makes things work more slowly... Right. The funny part with path_conv is that 2/3 of the storage is in fs_info. Is that really needed/useful? >Anyway, please feel free to check in what you have. OK, this evening. Pierre
Re: access () and path.cc
On Fri, Feb 28, 2003 at 01:02:58AM -0500, Pierre A. Humblet wrote: >At 12:56 AM 2/28/2003 -0500, you wrote: >>On Fri, Feb 28, 2003 at 12:49:59AM -0500, Pierre A. Humblet wrote: >>>OK, following Chris' remarks here is a much smaller set >>>of changes. >> >>Do you think it would make sense to do something along the lines >>of: >>>+ path_conv pc (cfd->is_device ? cfd->get_name () : >cfd->get_win32_name (), PC_SYM_NOFOLLOW); > >I guess one could but judging from the times I see in >strace it's not really justified. > >On the other hand that's something that we could look at after you >integrate your code. There could eventually be a single get_name >returning what's appropriate. This isn't an issue with my code, at least for fstat64. That's one of the reasons for my changes. I was trying to minimize the number of duplicate passes through path_conv::check. I still have some tweaking to do though since adding path_conv to fhandler balloons the sizes of fhandler_base and ends up using a lot more space in cygheap, which means more memory to copy on fork, which could mean slower forks, which means that my performance improvement makes things work more slowly... Anyway, please feel free to check in what you have. cgf
Re: access () and path.cc
At 12:56 AM 2/28/2003 -0500, you wrote: >On Fri, Feb 28, 2003 at 12:49:59AM -0500, Pierre A. Humblet wrote: >>OK, following Chris' remarks here is a much smaller set >>of changes. > >Do you think it would make sense to do something along the lines >of: >>+ path_conv pc (cfd->is_device ? cfd->get_name () : cfd->get_win32_name (), PC_SYM_NOFOLLOW); I guess one could but judging from the times I see in strace it's not really justified. On the other hand that's something that we could look at after you integrate your code. There could eventually be a single get_name returning what's appropriate. Pierre
Re: access () and path.cc
On Fri, Feb 28, 2003 at 12:56:35AM -0500, Christopher Faylor wrote: >On Fri, Feb 28, 2003 at 12:49:59AM -0500, Pierre A. Humblet wrote: >>OK, following Chris' remarks here is a much smaller set >>of changes. > >Do you think it would make sense to do something along the lines >of: > >>+ path_conv pc (cfd->is_device ? cfd->get_name () : cfd->get_win32_name (), >>PC_SYM_NOFOLLOW); cfd->is_device () Btw, thanks much for understanding about this. I appreciate that you were trying to fix this the right way the first time. cgf
Re: access () and path.cc
On Fri, Feb 28, 2003 at 12:49:59AM -0500, Pierre A. Humblet wrote:
>OK, following Chris' remarks here is a much smaller set
>of changes.
Do you think it would make sense to do something along the lines
of:
>+ path_conv pc (cfd->is_device ? cfd->get_name () : cfd->get_win32_name (),
>PC_SYM_NOFOLLOW);
just to minimize the (minimal) performance hit? Ordinarily, I
wouldn't advocate such a kludge but these are special circumstances.
I suppose you could do something similar in access(), too.
cgf
>Pierre
>
>
>2003-02-28 Pierre Humblet <[EMAIL PROTECTED]>
>
> * syscalls.cc (fstat64): Pass get_name () to pc.
> (access): Pass fn to stat_worker.
>
>Index: syscalls.cc
>===
>RCS file: /cvs/src/src/winsup/cygwin/syscalls.cc,v
>retrieving revision 1.246
>diff -u -p -r1.246 syscalls.cc
>--- syscalls.cc 21 Feb 2003 14:29:18 - 1.246
>+++ syscalls.cc 28 Feb 2003 05:46:09 -
>@@ -1013,7 +1013,7 @@ fstat64 (int fd, struct __stat64 *buf)
> res = -1;
> else
> {
>- path_conv pc (cfd->get_win32_name (), PC_SYM_NOFOLLOW);
>+ path_conv pc (cfd->get_name (), PC_SYM_NOFOLLOW);
> memset (buf, 0, sizeof (struct __stat64));
> res = cfd->fstat (buf, &pc);
> if (!res && cfd->get_device () != FH_SOCKET)
>@@ -1200,7 +1200,7 @@ access (const char *fn, int flags)
> return check_file_access (real_path, flags);
>
> struct __stat64 st;
>- int r = stat_worker (real_path, &st, 0);
>+ int r = stat_worker (fn, &st, 0);
> if (r)
> return -1;
> r = -1;
>
>
Re: access () and path.cc
OK, following Chris' remarks here is a much smaller set
of changes.
Pierre
2003-02-28 Pierre Humblet <[EMAIL PROTECTED]>
* syscalls.cc (fstat64): Pass get_name () to pc.
(access): Pass fn to stat_worker.
Index: syscalls.cc
===
RCS file: /cvs/src/src/winsup/cygwin/syscalls.cc,v
retrieving revision 1.246
diff -u -p -r1.246 syscalls.cc
--- syscalls.cc 21 Feb 2003 14:29:18 - 1.246
+++ syscalls.cc 28 Feb 2003 05:46:09 -
@@ -1013,7 +1013,7 @@ fstat64 (int fd, struct __stat64 *buf)
res = -1;
else
{
- path_conv pc (cfd->get_win32_name (), PC_SYM_NOFOLLOW);
+ path_conv pc (cfd->get_name (), PC_SYM_NOFOLLOW);
memset (buf, 0, sizeof (struct __stat64));
res = cfd->fstat (buf, &pc);
if (!res && cfd->get_device () != FH_SOCKET)
@@ -1200,7 +1200,7 @@ access (const char *fn, int flags)
return check_file_access (real_path, flags);
struct __stat64 st;
- int r = stat_worker (real_path, &st, 0);
+ int r = stat_worker (fn, &st, 0);
if (r)
return -1;
r = -1;
Re: access () and path.cc
On Thu, Feb 27, 2003 at 11:54:37PM -0500, Pierre A. Humblet wrote: >At 11:36 PM 2/27/2003 -0500, you wrote: >>Pierre, >>You and Corinna are giving me a headache. :-) > >My immediate access () problem can be fixed by replacing >real_path by fn in the stat_worker call. > >Problem #1 is a real bug, should be easy to fix, and may not >show up anywhere anyway, so not urgent. I've known about this for a while and I have never considered it a bug worth fixing. It should disappear in my branch since the code which causes it to happen should also disappear. Hmm. I guess I'd better make sure that's true now that I've said this. >Problems #2 & 3 can be solved by >>For instance, wouldn't all this be alleviated just by using >>cfg->get_name() rather than cfg->get_win32_name in the stat functions? >at the expense of efficiency (most of the time). >But efficiency can be revisited after you integrate your changes. Right. That call disappears in my code. >BTW, I was looking at the path/fhandler code and came to the tentative >conclusion that it shouldn't be necessary to keep both a Windows path >and a Unix path in the fhandler structures. >For the files understood by Windows, I think the Windows path is enough. >For the other files (most /dev, /proc), the Posix path should be OK. The path_conv structure is passed around in fhandler_base in my code so it's very different. I'd have to check but I think that devices always just show up as \dev\whatever. hash_path_name sort of relies on windows path names but that is not a huge deal. It may not even be applicable to devices. Please remind me about all of this when I merge my branch to the trunk. I think that some of these changes should probably be reverted in the name of efficiency at that time. cgf
Re: access () and path.cc
At 11:36 PM 2/27/2003 -0500, you wrote: >Pierre, >You and Corinna are giving me a headache. :-) My immediate access () problem can be fixed by replacing real_path by fn in the stat_worker call. Problem #1 is a real bug, should be easy to fix, and may not show up anywhere anyway, so not urgent. Problems #2 & 3 can be solved by >For instance, wouldn't all this be alleviated just by using >cfg->get_name() rather than cfg->get_win32_name in the stat functions? at the expense of efficiency (most of the time). But efficiency can be revisited after you integrate your changes. BTW, I was looking at the path/fhandler code and came to the tentative conclusion that it shouldn't be necessary to keep both a Windows path and a Unix path in the fhandler structures. For the files understood by Windows, I think the Windows path is enough. For the other files (most /dev, /proc), the Posix path should be OK. Pierre
Re: access () and path.cc
Pierre, You and Corinna are giving me a headache. :-) The code on my branch changes all of the device handling. The stat stuff is different, too. The changes that Corinna just made were very tough to integrate. Your changes will be too. In fact, I think everything you did will probably not be applicable to my branch. It doesn't seem like it will be worth the effort given that I expect to merge my changes into the trunk for 1.3.22. If there is no other way to do this, then I'll just hand tweak the stuff on my branch but if there is a stopgap measure that could be used, I would appreciate it if we could explore that. For instance, wouldn't all this be alleviated just by using cfg->get_name() rather than cfg->get_win32_name in the stat functions? cgf On Thu, Feb 27, 2003 at 11:04:53PM -0500, Pierre A. Humblet wrote: >The patch below needs careful review & probably work as I >don't know path.cc very well and you may have other ideas.
Re: access()
On Fri, Feb 21, 2003 at 05:33:45PM +0100, Corinna Vinschen wrote: >On Fri, Feb 21, 2003 at 10:32:36AM -0500, Christopher Faylor wrote: >> If I read Pierre's previous message correctly, it sounds like /bin/test >> is now broken. Was someone going to fix that? > >/bin/test as well as bash are *still* broken, so nothing has changed ;-) > >I'll upload a new bash soon after a 1.3.21 release. Who's maintaining >sh-utils? http://sources.redhat.com/cgi-bin/htsearch?words=sh-utils&restrict=%2Fml%2Fcygwin-announce%2F cgf
Re: access()
On Fri, Feb 21, 2003 at 10:32:36AM -0500, Christopher Faylor wrote: > If I read Pierre's previous message correctly, it sounds like /bin/test > is now broken. Was someone going to fix that? /bin/test as well as bash are *still* broken, so nothing has changed ;-) I'll upload a new bash soon after a 1.3.21 release. Who's maintaining sh-utils? Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Developermailto:[EMAIL PROTECTED] Red Hat, Inc.
Re: access()
Christopher Faylor wrote:
> If I read Pierre's previous message correctly, it sounds like /bin/test
> is now broken. Was someone going to fix that?
It's not any worse than before but not as good as sh, or (soon) bash.
I was going to mention this to the sh-utils maintainer :)
I can send a proper patch (where?), but essentially in test.c
+ #ifdef (__CYGWIN__)
+ #define eaccess(x,y) access(x,y)
+ #else
static int
eaccess (char *path, int mode)
{
struct stat st;
and a matching #endif
Pierre
Re: access()
Corinna Vinschen wrote:
> Applied.
Corinna,
I am still worried about using !real_path.exists() to determine
non existence, as done in several places in Cygwin.
That function checks if the file attributes are
After some experiments I found out that GetFileAttributes returns
FFF on an existing file if
a) the file ACL does not allow the caller to read the attributes
and
b) the directory is unreadable.
Do you agree or is it more complicated?
In principle this does not prevent the caller from reading the
security descriptor.
To return more accurate information, symlink_info::check could call
GetLastError. If it == 5, the file exists but there is an access problem.
What to do then is TBD.
If we take for granted that (with the existing code) the situation is
hopeless when the file attributes are F then the test
if (!pc->exists ())
{
debug_printf ("already determined that pc does not exist");
could be moved from fhandler_disk_file::fstat_by_name
to fhandler_disk_file::fstat (after the get_io_handle test).
While we are at it, set_query_open (query_open_already = true);
could also be called when a file has acls and ntsec is true.
On the other hand, if we keep the fstat code as it is, then for consistency
the following code in access
if (!real_path.exists ())
{
set_errno (ENOENT);
return -1;
}
if (!(flags & (R_OK | W_OK | X_OK)))
return 0;
should be weakened to
if (real_path.exists () && !(flags & (R_OK | W_OK | X_OK)))
return 0
so that we go ahead and try to read the sd even with !real_path.exists()
What you you think?
Pierre
Re: access()
On Fri, Feb 21, 2003 at 03:31:27PM +0100, Corinna Vinschen wrote: >On Thu, Feb 20, 2003 at 08:15:34PM -0500, Pierre A. Humblet wrote: >> However bash already uses access() when AFS is defined. Thus it >> would be a 1/2 line patch in bash (test.c and findcmd.c) to also >> use access() for Cygwin. >> - #if defined (AFS) >> + #if defined (AFS) || defined (__CYGWIN__) >> That would be a significant improvement, IMO. What do you think? > >Yes, I'll change that. Thanks for the hint. > >> 2003-02-21 Pierre Humblet <[EMAIL PROTECTED]> >> >> * autoload.cc (AccessCheck): Add. >> (DuplicateToken): Add. >> * security.h (check_file_access): Declare. >> * syscalls.cc (access): Convert path to Windows, check existence >> and readonly attribute. Call check_file_access instead of acl_access. >> * security.cc (check_file_access): Create. >> * sec_acl (acl_access): Delete. > >I'm impressed. Works nice with no more handcrafted messing around >with ACLs. If I read Pierre's previous message correctly, it sounds like /bin/test is now broken. Was someone going to fix that? cgf
Re: access()
On Thu, Feb 20, 2003 at 08:15:34PM -0500, Pierre A. Humblet wrote: > However bash already uses access() when AFS is defined. Thus it > would be a 1/2 line patch in bash (test.c and findcmd.c) to also > use access() for Cygwin. > - #if defined (AFS) > + #if defined (AFS) || defined (__CYGWIN__) > That would be a significant improvement, IMO. What do you think? Yes, I'll change that. Thanks for the hint. > 2003-02-21 Pierre Humblet <[EMAIL PROTECTED]> > > * autoload.cc (AccessCheck): Add. > (DuplicateToken): Add. > * security.h (check_file_access): Declare. > * syscalls.cc (access): Convert path to Windows, check existence > and readonly attribute. Call check_file_access instead of acl_access. > * security.cc (check_file_access): Create. > * sec_acl (acl_access): Delete. I'm impressed. Works nice with no more handcrafted messing around with ACLs. Applied. Thanks, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Developermailto:[EMAIL PROTECTED] Red Hat, Inc.
Re: access()
On Thu, Feb 20, 2003 at 08:15:34PM -0500, Pierre A. Humblet wrote: >2) I am not sure when to use LoadDLLfuncEx vs. LoadDLLfunc. LoadDLLfunc issues an error if a function isn't found. LoadDllFuncEx lets you return an error code when the function isn't found. cgf
