bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Patch against src/remove.c for correct handling of negative error co


From: Jim Meyering
Subject: Re: Patch against src/remove.c for correct handling of negative error codes
Date: Mon, 31 Mar 2008 15:01:13 +0200

"Axel Dörfler" <address@hidden> wrote:
> the enclosed patch changes the following (remove.c against HEAD):
...

Thank you for the patch.
Sorry not to have replied sooner.
At first I hesitated to make a relatively invasive change
to remove.c solely to accommodate a fringe target like BeOS/Haiku.
Then I noticed the change wasn't quite right, and finally I
discovered a real bug in rm (just fixed, separately).

> diff --git a/src/remove.c b/src/remove.c
...
So far, so good.

> @@ -839,15 +842,18 @@ prompt (int fd_cwd, Dirstack_state const *ds, char 
> const *filename,
>    if (x->interactive == RMI_NEVER)
>      return RM_OK;
>
> +  errno = 0;
> +
>    if (!x->ignore_missing_files
>        && ((x->interactive == RMI_ALWAYS) || x->stdin_tty)
>        && dirent_type != DT_LNK)
>      write_protected = write_protected_non_symlink (fd_cwd, filename, ds, 
> sbuf);
>
> -  if (write_protected || x->interactive == RMI_ALWAYS)
> +  if (write_protected || errno || x->interactive == RMI_ALWAYS)
>      {
> -      if (write_protected <= 0 && dirent_type == DT_UNKNOWN)
> +      if (errno == 0 && dirent_type == DT_UNKNOWN)
>       {
> +       /* Might fail, e.g., for `rm '''.  */
>         if (cache_fstatat (fd_cwd, filename, sbuf, AT_SYMLINK_NOFOLLOW) == 0)
>           {
>             if (S_ISLNK (sbuf->st_mode))
> @@ -857,14 +863,9 @@ prompt (int fd_cwd, Dirstack_state const *ds, char const 
> *filename,
>             /* Otherwise it doesn't matter, so leave it DT_UNKNOWN.  */
>             *pdirent_type = dirent_type;
>           }
> -       else
> -         {
> -           /* This happens, e.g., with `rm '''.  */
> -           write_protected = errno;
> -         }
>       }
>
> -      if (write_protected <= 0)
> +      if (errno == 0)

However, at this point we can no longer rely on errno to have
retained its value from the call to write_protected_non_symlink.
It may have been changed via the cache_fstatat call.

So I reworked things so that the errno value is saved
in its own variable, and changed several conditions to
make it clearer that the main artifact of this change is
the return-value change of sign.

Here's the patch I expect to push:

>From: Jim Meyering <address@hidden>
Date: Mon, 24 Mar 2008 17:38:27 +0100
Subject: [PATCH] remove.c: accommodate systems with negative errno values

This is required at least on Haiku and BeOS.
* src/remove.c (write_protected_non_symlink): Return 1 for a write-
protected non-symlink, 0 if we determine it's not, and -1 upon
error (setting errno accordingly only in this final case).
(prompt): Deal with the changed semantics of the above function.
Based on this patch from Axel Dörfler:
http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/13071

Signed-off-by: Jim Meyering <address@hidden>
---
 THANKS       |    1 +
 src/remove.c |   38 +++++++++++++++++++++++++-------------
 2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/THANKS b/THANKS
index d38e51f..d746b29 100644
--- a/THANKS
+++ b/THANKS
@@ -21,6 +21,7 @@ Albert Hopkins                      address@hidden
 Alberto Accomazzi                   address@hidden
 aldomel                             address@hidden
 Alen Muzinic                        address@hidden
+Axel Dörfler                        address@hidden
 Alexandre Duret-Lutz                address@hidden
 Alexey Solovyov                     address@hidden
 Alexey Vyskubov                     address@hidden
diff --git a/src/remove.c b/src/remove.c
index 7ba1e38..07c2f71 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -728,9 +728,9 @@ AD_is_removable (Dirstack_state const *ds, char const *file)
   return ! (top->unremovable && hash_lookup (top->unremovable, file));
 }

-/* Return -1 if FILE is an unwritable non-symlink,
+/* Return 1 if FILE is an unwritable non-symlink,
    0 if it is writable or some other type of file,
-   a positive error number if there is some problem in determining the answer.
+   -1 and set errno if there is some problem in determining the answer.
    Set *BUF to the file status.
    This is to avoid calling euidaccess when FILE is a symlink.  */
 static int
@@ -742,7 +742,7 @@ write_protected_non_symlink (int fd_cwd,
   if (can_write_any_file ())
     return 0;
   if (cache_fstatat (fd_cwd, file, buf, AT_SYMLINK_NOFOLLOW) != 0)
-    return errno;
+    return -1;
   if (S_ISLNK (buf->st_mode))
     return 0;
   /* Here, we know FILE is not a symbolic link.  */
@@ -799,15 +799,18 @@ write_protected_non_symlink (int fd_cwd,
       = obstack_object_size (&ds->dir_stack) + strlen (file);

     if (MIN (PATH_MAX, 8192) <= file_name_len)
-      return euidaccess_stat (buf, W_OK) ? 0 : -1;
+      return ! euidaccess_stat (buf, W_OK);
     if (euidaccess (xfull_filename (ds, file), W_OK) == 0)
       return 0;
     if (errno == EACCES)
-      return -1;
+      {
+       errno = 0;
+       return 1;
+      }

     /* Perhaps some other process has removed the file, or perhaps this
        is a buggy NFS client.  */
-    return errno;
+    return -1;
   }
 }

@@ -839,14 +842,19 @@ prompt (int fd_cwd, Dirstack_state const *ds, char const 
*filename,
   if (x->interactive == RMI_NEVER)
     return RM_OK;

+  int wp_errno = 0;
+
   if (!x->ignore_missing_files
       && ((x->interactive == RMI_ALWAYS) || x->stdin_tty)
       && dirent_type != DT_LNK)
-    write_protected = write_protected_non_symlink (fd_cwd, filename, ds, sbuf);
+    {
+      write_protected = write_protected_non_symlink (fd_cwd, filename, ds, 
sbuf);
+      wp_errno = errno;
+    }

   if (write_protected || x->interactive == RMI_ALWAYS)
     {
-      if (write_protected <= 0 && dirent_type == DT_UNKNOWN)
+      if (0 <= write_protected && dirent_type == DT_UNKNOWN)
        {
          if (cache_fstatat (fd_cwd, filename, sbuf, AT_SYMLINK_NOFOLLOW) == 0)
            {
@@ -860,11 +868,12 @@ prompt (int fd_cwd, Dirstack_state const *ds, char const 
*filename,
          else
            {
              /* This happens, e.g., with `rm '''.  */
-             write_protected = errno;
+             write_protected = -1;
+             wp_errno = errno;
            }
        }

-      if (write_protected <= 0)
+      if (0 <= write_protected)
        switch (dirent_type)
          {
          case DT_LNK:
@@ -875,15 +884,18 @@ prompt (int fd_cwd, Dirstack_state const *ds, char const 
*filename,

          case DT_DIR:
            if (!x->recursive)
-             write_protected = EISDIR;
+             {
+               write_protected = -1;
+               wp_errno = EISDIR;
+             }
            break;
          }

       char const *quoted_name = quote (full_filename (filename));

-      if (0 < write_protected)
+      if (write_protected < 0)
        {
-         error (0, write_protected, _("cannot remove %s"), quoted_name);
+         error (0, wp_errno, _("cannot remove %s"), quoted_name);
          return RM_ERROR;
        }

--
1.5.5.rc1.13.g79388




reply via email to

[Prev in Thread] Current Thread [Next in Thread]