Package: coreutils;
Reported by: "Joachim Schmitz" <jojo <at> schmitz-digital.de>
Date: Thu, 15 Dec 2011 14:08:01 UTC
Severity: wishlist
Tags: notabug
Message #126 received at 10305 <at> debbugs.gnu.org (full text, mbox):
From: "Joachim Schmitz" <jojo <at> schmitz-digital.de> To: "'Paul Eggert'" <eggert <at> cs.ucla.edu> Cc: 10305 <at> debbugs.gnu.org, 'Eric Blake' <eblake <at> redhat.com>, bug-gnulib <at> gnu.org, 'Jim Meyering' <jim <at> meyering.net> Subject: RE: bug#10305: coreutils-8.14, "rm -r" fails with EBADF Date: Wed, 27 Jun 2012 09:25:22 +0200
> From: Paul Eggert [mailto:eggert <at> cs.ucla.edu]
> Sent: Wednesday, June 27, 2012 2:49 AM
> To: Joachim Schmitz
> Cc: 10305 <at> debbugs.gnu.org; bug-gnulib <at> gnu.org; 'Eric Blake'; 'Jim Meyering'
> Subject: Re: bug#10305: coreutils-8.14, "rm -r" fails with EBADF
>
> On 06/26/2012 09:38 AM, Joachim Schmitz wrote:
>
> > Let me know what you think and where/how you'd do it differently.
>
> The changes mostly look good. The trivial ones we've incorporated already. I
> have some comments on the nontrivial ones (please see below).
> But before we get into it too much further, are you and your company willing to
> assign copyright to your nontrival changes to the FSF?
> If so, I can send you more info about how to do that.
I'd need to check, will pursue this in a different email.
> Here are some comments about that patch:
>
>
> > --- ./configure.orig 2011-03-12 03:50:18.000000000 -0600
> > +++ ./configure 2012-06-26 06:49:17.000000000 -0500
> For future versions of this patch there's no need to show differences in
> automatically-generated files like 'configure'.
That change then needs to go into m4/dirfd.m4, right?
> > --- ./gnu/argp-help.c.orig 2011-03-12 03:14:26.000000000 -0600
> > +++ ./gnu/argp-help.c 2011-06-16 02:01:23.000000000 -0500
>
> This one Bruno just now fixed in a different way:
> http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=2457d7ca685
> 6f84502b09fa4690f6f4187de050f
Fine by me
> > --- ./gnu/regex.c.orig 2011-03-12 03:14:32.000000000 -0600
> > +++ ./gnu/regex.c 2011-06-17 04:07:16.000000000 -0500
>
> This one we also fixed in a different way:
> http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=d4903bb0efa
> c5e399b785c71367d8cda3fb539ab
Fine too.
> > --- ./gnu/dirfd.c.orig 2011-03-12 03:14:28.000000000 -0600
> > +++ ./gnu/dirfd.c 2012-06-25 02:55:09.000000000 -0500
> > ...
> > +#ifdef __TANDEM
> > +# include <unistd.h> /* for _gl_fnum2dt(), needed in C99 mode */
> > +#endif
>
> Shouldn't that be "_gl_fnum2fd"?
Yes, of course, stupid typo.
> More important, doesn't the declaration of _gl_fnum2fd belong better in
> dirent.h, not unistd.h? Among other things, that would mean the above change
> can be omitted.
My attempts to integrate this into coreutils-8.17 seem to indicate that this function and a couple more are used elsewhere too
Looks like there closedir.o needs _gl_ungerister_fnum(), dirfd.c needs _gl_fnum2ds() and opendir.c needs _gl_register_fnum().
> > int
> > dirfd (DIR *dir_p)
> > {
> > int fd = DIR_TO_FD (dir_p);
> > if (fd == -1)
> > errno = ENOTSUP;
> > +#ifdef __TANDEM
> > + fd = _gl_fnum2fd(fd);
> > +#endif
>
> This might be cleaner if DIR_TO_FD invoked _gl_fnum2fd directly.
> That way, dirfd.c could be left alone. (Or perhaps not; I don't understand the
> code that well.)
Won't that make that macro too complicated?
> > +# define NOFNUM -1
>
> What's this for? I don't see it used anywhere.
Indeed, scratch it.
> > +# define NOFD -1
>
> Body needs to be parenthesized.
Point taken.
> > + char fnum; /* 'y' or 'n', actually a bool */
>
> Why not use 1 and 0? That's far more typical for boolean values, and
> generates better code.
True. But the corresponding member in dir_info_t should remain a char (or signed char?), to save space, shouldn't it?
Maybe we could also make it a short and place fnum right there? That would change the implementation quite a bit though, but might make it leaner too. And the comment
/* FIXME - add a DIR* member to make dirfd possible on mingw? */
Indicates that there is a change pending for a similar purpose on a different platform...
> > +#ifdef __TANDEM
> > + || (stat (filename, &statbuf) == 0 && S_ISDIR
> > +(statbuf.st_mode)))
>> +#else
> > || (fstat (fd, &statbuf) == 0 && S_ISDIR (statbuf.st_mode)))
> > +#endif
>
> fstat doesn't work on Tandem? I must be missing something here.
This is a special case for those "dummy directories" from the patch chunk below.
And yes, that warrants a comment in the above patch chunk ;-)
> > fd = orig_open (filename, flags, mode);
> > +#ifdef __TANDEM
> > + /* On NonStop open(2) can open an OSS directory, but not /G & /E
> > + * directory, hence we do a dummy open here and override fstat() in
> > + * fchdir.c to hide the fact that we have a dummy.
> > + */
> > + if (fd < 0 && errno == EISDIR)
> > + fd = open ("/dev/null", flags, mode);
> > +#endif
>
> If 'flags' contains O_WRONLY or O_RDWR, this will misbehave on an OSS
> directory, since open will fail with errno == EISDIR but we don't want to replace
> it with /dev/null. So the fallback needs to be conditioned on the file being
> opened read-only.
These cases are handeld furter above in the (existing) code:
if (flags & (O_CREAT | O_WRONLY | O_RDWR))
{
size_t len = strlen (filename);
if (len > 0 && filename[len - 1] == '/')
{
errno = EISDIR;
return -1;
}
}
#endif
fd = orig_open (filename, flags, mode);
#ifdef __TANDEM
/* On NonStop open(2) can open an OSS directory, but not /G & /E
* directory, hence we do a dummy open here and override fstat() in
* fchdir.c to hide the fact that we have a dummy.
*/
if (fd < 0 && errno == EISDIR)
fd = open ("/dev/null", flags, mode);
#endif
> > --- ./gnu/unlinkdir.c.orig 2011-03-12 03:14:34.000000000 -0600
> > +++ ./gnu/unlinkdir.c 2012-06-26 08:46:41.000000000 -0500
> > ...
> > --- ./src/extract.c.orig 2010-11-27 04:33:22.000000000 -0600
> > +++ ./src/extract.c 2011-06-16 01:55:46.000000000 -0500
>
> I just now fixed this in a different way in gnulib and tar master.
Good. Where does that ROOT_UID get set?
> > /usr/local/bin/diff -EBbu ./tests/genfile.c.orig ./tests/genfile.c
> > --- ./tests/genfile.c.orig 2010-10-24 13:06:45.000000000 -0500
> > +++ ./tests/genfile.c 2011-06-16 01:55:46.000000000 -0500
> > @@ -610,9 +610,17 @@
> > else if (strcmp (p, "size") == 0)
> > printf ("%s", umaxtostr (st.st_size, buf));
> > else if (strcmp (p, "blksize") == 0)
> > +#ifdef __TANDEM
> > + printf ("*****Nothing to print on NonStop for st_blksize****\n");
> > +#else
> > printf ("%s", umaxtostr (st.st_blksize, buf));
> > +#endif
> > else if (strcmp (p, "blocks") == 0)
> > +#ifdef __TANDEM
> > + printf ("*****Nothing to print on NonStop for st_blocks****\n");
> > +#else
> > printf ("%s", umaxtostr (st.st_blocks, buf));
> > +#endif
> > else if (strcmp (p, "atime") == 0)
> > printf ("%lu", (unsigned long) st.st_atime);
> > else if (strcmp (p, "atimeH") == 0)
>
> I assume st_blksize and st_blocks are garbage on Tandem?
> Is there any harm in printing the garbage?
No, we don't have those struct members at all. Instead of #ifdef __TANDEM it should probably better be #if HAVE_STAT_ST_BLOCKS and #if HAVE_STRUCT_STAT_ST_BLKSIZE or some such and remain silent if not, maybe like this:
else if (strcmp (p, "blksize") == 0)
#if HAVE_STRUCT_STAT_ST_BLKSIZE
printf ("%s", umaxtostr (st.st_blksize, buf));
#endif
else if (strcmp (p, "blocks") == 0)
#if HAVE_STRUCT_STAT_ST_BLOCKS
printf ("%s", umaxtostr (st.st_blocks, buf));
#endif
Bye, Jojo
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.