[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#10305: coreutils-8.14, "rm -r" fails with EBADF
From: |
Joachim Schmitz |
Subject: |
bug#10305: coreutils-8.14, "rm -r" fails with EBADF |
Date: |
Wed, 27 Jun 2012 09:25:22 +0200 |
> From: Paul Eggert [mailto:address@hidden
> Sent: Wednesday, June 27, 2012 2:49 AM
> To: Joachim Schmitz
> Cc: address@hidden; address@hidden; '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