Re: bug#10305: coreutils-8.14, "rm -r" fails with EBADF

[Paul Eggert]

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.

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'.

> --- ./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=2457d7ca6856f84502b09fa4690f6f4187de050f

> --- ./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=d4903bb0efac5e399b785c71367d8cda3fb539ab

> --- ./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"?

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.

>  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.)

> +# define NOFNUM      -1

What's this for?  I don't see it used anywhere.

> +# define NOFD        -1

Body needs to be parenthesized.

> +  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.

> +#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.

>    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.

> --- ./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.

> /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?