> From: Paul Eggert [mailto:egg...@cs.ucla.edu] > Sent: Wednesday, June 27, 2012 2:49 AM > To: Joachim Schmitz > Cc: 10...@debbugs.gnu.org; bug-gnulib@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
