[1.7] rename/renameat error

[1.7] rename/renameat error

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

POSIX states that rename must fail with EINVAL if either argument ends in
'.' or '..' (after trailing slashes are stripped).  Cygwin 1.7 is
detecting this situation (which is a step up from 1.5 which did the rename
anyways), but sets errno to EBUSY instead of EINVAL.

$ mkdir d1 d2
$ mv -T d1/. d2 # the -T forces a raw rename("d1/.","d2")
mv: cannot move `d1/.' to `d2': Device or resource busy

- --
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@byu.net
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqlK14ACgkQ84KuGfSFAYDdDwCfYSdpMnuGmEM9b6ux4vTRJV9I
UkAAnR6R7WPbGfgYr31xDxAxecWOu3BD
=O2KC
-----END PGP SIGNATURE-----

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: [1.7] rename/renameat error

[Corinna Vinschen]

On Sep  7 09:48, Eric Blake wrote:
> POSIX states that rename must fail with EINVAL if either argument ends in
> '.' or '..' (after trailing slashes are stripped).  Cygwin 1.7 is
> detecting this situation (which is a step up from 1.5 which did the rename
> anyways), but sets errno to EBUSY instead of EINVAL.

Thanks for catching.  Feel free to fix the rename function accordingly.
This falls definitely under the trivial patch rule.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: [1.7] rename/renameat error

[Eric Blake]

Corinna Vinschen <corinna-cygwin <at> cygwin.com> writes:

> > POSIX states that rename must fail with EINVAL if either argument ends in
> > '.' or '..' (after trailing slashes are stripped).  Cygwin 1.7 is
> > detecting this situation (which is a step up from 1.5 which did the rename
> > anyways), but sets errno to EBUSY instead of EINVAL.
> 
> Thanks for catching.  Feel free to fix the rename function accordingly.

OK, I'll look into it (I don't know how large the patch will be, yet).

> This falls definitely under the trivial patch rule.

Trivial or not, I won't commit it until the copyright papers are final (I've 
made enough "trivial" contributions over the years to add up to a non-trivial 
sum total).  But the good news: I finally got my employer to sign forms today, 
so the only delay now is for the postal service.  Let's see, now, how long 
since you first asked me to fill out forms?

-- 
Eric Blake




--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: [1.7] rename/renameat error

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> > > POSIX states that rename must fail with EINVAL if either argument ends in
> > > '.' or '..' (after trailing slashes are stripped).  Cygwin 1.7 is
> > > detecting this situation (which is a step up from 1.5 which did the rename
> > > anyways), but sets errno to EBUSY instead of EINVAL.
> > 
> > Thanks for catching.  Feel free to fix the rename function accordingly.
> 
> OK, I'll look into it (I don't know how large the patch will be, yet).

And link("a","f/.") should not create "f" as a regular file, either.  I'm still 
looking at where to patch things.

Also, we currently allow link("a","b") on FAT, but it might be nicer to fail 
with EPERM on file systems where hard links are not supported, to match Linux 
behavior (portable programs, like autoconf, already have fallbacks to perform 
cp if linking fails, but the copy should be done by the caller, not by link() 
itself).

-- 
Eric Blake




--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: [1.7] rename/renameat error

[Christopher Faylor]

On Wed, Sep 09, 2009 at 04:36:39PM +0000, Eric Blake wrote:
>Eric Blake <ebb9 <at> byu.net> writes:
>>>>POSIX states that rename must fail with EINVAL if either argument ends
>>>>in '.' or '..' (after trailing slashes are stripped).  Cygwin 1.7 is
>>>>detecting this situation (which is a step up from 1.5 which did the
>>>>rename anyways), but sets errno to EBUSY instead of EINVAL.
>>>
>>>Thanks for catching.  Feel free to fix the rename function accordingly.
>>
>>OK, I'll look into it (I don't know how large the patch will be, yet).
>
>And link("a","f/.") should not create "f" as a regular file, either.
>I'm still looking at where to patch things.

Argh.  That's a longstanding problem with brain-dead windows behavior.  It's
supposed to be handled in path_conv::check, IIRC.

>Also, we currently allow link("a","b") on FAT, but it might be nicer to fail 
>with EPERM on file systems where hard links are not supported, to match Linux 
>behavior (portable programs, like autoconf, already have fallbacks to perform 
>cp if linking fails, but the copy should be done by the caller, not by link() 
>itself).

We've debated this over the years but I'm ok with not lying to the caller about
performing a link when we really didn't.

cgf

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: [1.7] rename/renameat error

[Corinna Vinschen]

On Sep  9 13:42, Christopher Faylor wrote:
> On Wed, Sep 09, 2009 at 04:36:39PM +0000, Eric Blake wrote:
> >Eric Blake <ebb9 <at> byu.net> writes:
> >>>>POSIX states that rename must fail with EINVAL if either argument ends
> >>>>in '.' or '..' (after trailing slashes are stripped).  Cygwin 1.7 is
> >>>>detecting this situation (which is a step up from 1.5 which did the
> >>>>rename anyways), but sets errno to EBUSY instead of EINVAL.
> >>>
> >>>Thanks for catching.  Feel free to fix the rename function accordingly.
> >>
> >>OK, I'll look into it (I don't know how large the patch will be, yet).
> >
> >And link("a","f/.") should not create "f" as a regular file, either.
> >I'm still looking at where to patch things.
> 
> Argh.  That's a longstanding problem with brain-dead windows behavior.  It's
> supposed to be handled in path_conv::check, IIRC.
> 
> >Also, we currently allow link("a","b") on FAT, but it might be nicer to fail 
> >with EPERM on file systems where hard links are not supported, to match Linux 
> >behavior (portable programs, like autoconf, already have fallbacks to perform 
> >cp if linking fails, but the copy should be done by the caller, not by link() 
> >itself).
> 
> We've debated this over the years but I'm ok with not lying to the caller about
> performing a link when we really didn't.

Yes, I guess it's really time to do that, considering that NTFS is now
the default filesystem for years anyway.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: [1.7] rename/renameat error

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> > > > Cygwin 1.7 is
> > > > detecting this situation (which is a step up from 1.5 which did the 
rename
> > > > anyways), but sets errno to EBUSY instead of EINVAL.
> > > 
> > > Thanks for catching.  Feel free to fix the rename function accordingly.
> > 
> > OK, I'll look into it (I don't know how large the patch will be, yet).
> 
> And link("a","f/.") should not create "f" as a regular file, either.  I'm 
still 
> looking at where to patch things.

I've got a patch in testing for both of these issues.  But while looking at 
path.cc, I've noticed a couple of things:

The code doesn't do a very good job of remembering lengths it has already 
seen.  For example, with relative paths, the code in normalize_posix_path does 
cwd.get, then strchr; it seems like since cwd.get already knows how many bytes 
it copied, that a simple API modification would pass that information back to 
the caller so that the caller doesn't have to use strchr to find the end of the 
string.  Anything we can do to avoid rescanning strings of known length will 
provide speedups in path handling.

I'm also wondering whether it is time to finally emulate Linux by requiring 
that when doing pathname resolution of 'a/..', that 'a' actually exist (either 
as a directory or a symlink to directory), instead of silently ignoring that 
part of the string.  Should I go ahead and spend time working up a patch for 
this?

-- 
Eric Blake



--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: [1.7] rename/renameat error

[Corinna Vinschen]

On Sep 22 21:02, Eric Blake wrote:
> Eric Blake <ebb9 <at> byu.net> writes:
> 
> > > > > Cygwin 1.7 is
> > > > > detecting this situation (which is a step up from 1.5 which did the 
> rename
> > > > > anyways), but sets errno to EBUSY instead of EINVAL.
> > > > 
> > > > Thanks for catching.  Feel free to fix the rename function accordingly.
> > > 
> > > OK, I'll look into it (I don't know how large the patch will be, yet).
> > 
> > And link("a","f/.") should not create "f" as a regular file, either.  I'm 
> still 
> > looking at where to patch things.
> 
> I've got a patch in testing for both of these issues.  But while looking at 
> path.cc, I've noticed a couple of things:
> 
> The code doesn't do a very good job of remembering lengths it has already 
> seen.  For example, with relative paths, the code in normalize_posix_path does 
> cwd.get, then strchr; it seems like since cwd.get already knows how many bytes 
> it copied, that a simple API modification would pass that information back to 
> the caller so that the caller doesn't have to use strchr to find the end of the 
> string.  Anything we can do to avoid rescanning strings of known length will 
> provide speedups in path handling.

This one might be worth a shot, if it's an easy patch.

> I'm also wondering whether it is time to finally emulate Linux by requiring 
> that when doing pathname resolution of 'a/..', that 'a' actually exist (either 
> as a directory or a symlink to directory), instead of silently ignoring that 
> part of the string.  Should I go ahead and spend time working up a patch for 
> this?

This is something which I have on my TODO list for a long time, but I
never saw a simple way to implement this without losing a lot of the
already lousy performance of the path conversion.  Additionally the path
conversion code is already quite complicated and I fear the unwanted
side effects such a change could have.  Therefore, I tend to think of
this as a welcome post-1.7.1 change.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: [1.7] rename/renameat error

[Christopher Faylor]

On Wed, Sep 23, 2009 at 11:22:09AM +0200, Corinna Vinschen wrote:
>On Sep 22 21:02, Eric Blake wrote:
>>Eric Blake <ebb9 <at> byu.net> writes:
>>
>>>>>>Cygwin 1.7 is detecting this situation (which is a step up from 1.5
>>>>>>which did the
>>rename
>>>>>>anyways), but sets errno to EBUSY instead of EINVAL.
>>>>>
>>>>>Thanks for catching.  Feel free to fix the rename function accordingly.
>>>>
>>>>OK, I'll look into it (I don't know how large the patch will be, yet).
>>>
>>>And link("a","f/.") should not create "f" as a regular file, either.
>>>I'm still looking at where to patch things.
>>
>>I've got a patch in testing for both of these issues.  But while
>>looking at path.cc, I've noticed a couple of things:
>>
>>The code doesn't do a very good job of remembering lengths it has
>>already seen.  For example, with relative paths, the code in
>>normalize_posix_path does cwd.get, then strchr; it seems like since
>>cwd.get already knows how many bytes it copied, that a simple API
>>modification would pass that information back to the caller so that the
>>caller doesn't have to use strchr to find the end of the string.
>>Anything we can do to avoid rescanning strings of known length will
>>provide speedups in path handling.
>
>This one might be worth a shot, if it's an easy patch.
>
>>I'm also wondering whether it is time to finally emulate Linux by
>>requiring that when doing pathname resolution of 'a/..', that 'a'
>>actually exist (either as a directory or a symlink to directory),
>>instead of silently ignoring that part of the string.  Should I go
>>ahead and spend time working up a patch for this?
>
>This is something which I have on my TODO list for a long time, but I
>never saw a simple way to implement this without losing a lot of the
>already lousy performance of the path conversion.  Additionally the
>path conversion code is already quite complicated and I fear the
>unwanted side effects such a change could have.  Therefore, I tend to
>think of this as a welcome post-1.7.1 change.

Can we discuss these types of things in cygwin-developers, please?  It
makes searching for decisions/discussions marginally more effective if
we discuss design decisions there.

I do think, however, that before we start making changes "for efficiency"
it might be time to start thinking about doing some performance tests on
the DLL.  But that's after 1.7.1 is released.

cgf

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple