[Toybox] [PATCH] mkpathat: return EEXIST only if MKPATHAT_MKLAST is given

[Yi-yo Chiang]

On Mon, Feb 8, 2021 at 3:41 PM Rob Landley <rob at landley.net> wrote:

> On 2/7/21 10:07 AM, Yi-Yo Chiang via Toybox wrote:
> > There was a regression:
> >
> > $ mkdir a
> > $ touch a/b
>
> I just tried that and it worked with no error?
>

I should've been more clear. I mean after doing
$ touch "a/b"
"a/b" is now a regular file, and then subsequent calls of mkpath("a/b")
would return 1 with errno EEXIST.
mkdir or touch itself doesn't error.


> > Calling mkpath("a/b") would return an error with EEXIST errno. This is
> > because "a/b" is a regular file. However mkpath() should only create the
> > leading directories of "a/b", which is "a/", which shouldn't be an error
> > because attempting to create an already existed directory shouldn't fail.
> >
> > ---
> >  lib/lib.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/lib.c b/lib/lib.c
> > index dfa4499f..76726931 100644
> > --- a/lib/lib.c
> > +++ b/lib/lib.c
> > @@ -177,7 +177,8 @@ int mkpathat(int atfd, char *dir, mode_t lastmode,
> int flags)
> >    // not-a-directory along the way, but the last one we must explicitly
> >    // test for. Might as well do it up front.
> >
> > -  if (!fstatat(atfd, dir, &buf, 0) && !S_ISDIR(buf.st_mode)) {
> > +  if (flags&MKPATHAT_MKLAST && !fstatat(atfd, dir, &buf, 0)
> > +      && !S_ISDIR(buf.st_mode)) {
> >      errno = EEXIST;
> >      return 1;
> >    }
>
> For reference, a test that demonstrates the failure is:
>
>   $ ./toybox install -D README a/b
>   install: -D 'a/b': File exists
>
>
Another repro step is with cpio. I (well my coworker really) were doing
something like this:
$ mkdir a
$ touch a/b
$ find a | cpio -o >a.cpio
$ cpio -id <a.cpio
cpio: a: File exists
cpio: mkpath 'a/b': File exists


And yes, I need to create and populate an install.tests, but there's a
> structural problem ("make install" is an existing make target so can't be
> used
> for the install command, which also knocks out test_install...) It's on
> the todo
> list.. :)
>

I was hesitant to attach a testcase demonstrated by "cpio", as I don't
think cpio is the real cause here. It just happens so cpio were calling
mkpath in the correct order.
If only we had a test case that directly test lib/lib.c itself?


>
> Committed a slightly different fix, with test. (If we successfully statted
> the
> target and don't mind that it's a file, return success immediately...)
>

Sounds good! If stat() succeeded, then no need to create anything.


>
> Thanks,
>
> Rob
>


-- 

Yi-yo Chiang
Software Engineer
yochiang at google.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20210208/3da129e3/attachment.html>