[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: bug#10305: coreutils-8.14, "rm -r" fails with EBADF
From: |
Paul Eggert |
Subject: |
Re: bug#10305: coreutils-8.14, "rm -r" fails with EBADF |
Date: |
Tue, 26 Jun 2012 17:48:56 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120615 Thunderbird/13.0.1 |
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?