bug#6555: stat enhancement

[Jim Meyering]

A Burgie wrote:
> On Mon, Jul 5, 2010 at 07:50, Jim Meyering <address@hidden> wrote:
>> A Burgie wrote:
>>> On Mon, Jul 5, 2010 at 06:48, Jim Meyering <address@hidden> wrote:
>>>>> The idea seems sensible.
>>>>
>>>> I think so, too.
>>>> However, the patch that adds the option would do well to add
>>>> a test that exercises it and to mention it in the NEWS file.
>>>> Your change will qualify as "significant".
>>>> Can you file a copyright assignment?  Here are guidelines:
>>>>
>>>>    http://git.savannah.gnu.org/cgit/coreutils.git/tree/HACKING#n327
>>>
>>> I was wondering if that would apply, but I did not think it would as
>>> it was not really my own code (I copied from previously-GPL'd code and
>>> only, perhaps, added three or four lines of my own).  Please confirm
>>> if I am mistaken on this.
>>
>> I figured that between moving that function into a file on its
>> own, declaring the function in its own header file,
>> adjusting df.c and stat.c to include the new .h file,
>> adjusting src/Makefile.am to list the new file, adding a test and
>> adding to NEWS you would end up adding more than 10 or 15 lines.
>
> Ah.... I see.  Very well; I'll get to work on the legal side of things.
>
>> Oh.  And documentation.  You'll want to add a line or two to
>> the section on stat in doc/coreutils.texi.
>
> Consider it done.
>
>> If it's too much work on the portability front (wouldn't surprise me),
>> it may be ok simply to put the statically-declared function body
>> in a new .c file and include the .c file from each of stat.c and df.c.
>
> I'll work to do it the right way and let you know if I fail there.
>
>> BTW, please adjust this part of your patch not to dereference NULL
>> when find_mount_point fails:
>>
>>  +    case 'm':
>>  +      out_string (pformat, prefix_len, find_mount_point (filename, 
>> statbuf));
>
> Any pointers (pun intended) on the best way to do that?  I'll check
> the df.c file to see what it does but if it's not there and you have a
> better option that'd be appreciated.  I'm not sure it's been pointed
> out yet, but I'm not a C expert yet.

Hmm...  Handling failure is actually looking complicated.
If someone used the %m format and find_mount_point fails,
then stat should exit nonzero, but the use of the new find_mount_point
would be down in print_stat, which is a void function, and which is passed
as a callback to print_it, which is also a void function.
Doing this cleanly would mean modifying signatures of at least
print_it, print_stat and print_statfs to return a success/fail
indicator to do_statfs.

So I've just adjusted those interfaces and added some comments.
Patch below.  That should make it easier for you.

Note the out_file_context function.
It currently prints "?" (and a diagnostic to stderr) when
it fails to determine a context string.  Such a failure
did not make stat itself exit nonzero, but with this patch,
now it does.

Your change should be sure to do the same.

>> Also, I'm a little reluctant to change the default format to
>> add an entire new line just for "Mountpoint: %s\n".
>> There is value in not changing the number of lines in the default output:
>> compatibility.
>
> Agreed.  My reasoning in doing so was perhaps a bit shortsighted.
> Most of the stat output seems to fit nicely within eighty columns and
> I did not want to disturb that too much.  Some lines could easily go
> outside this (File, Permissions with user/group names, etc.) but File
> was on the first line and I was concerned about current users who may
> be running stat and then piping output through 'head' or 'tail' to get
> just a portion of it.  I could easily enough add Mountpoint after the
> filename or anywhere else you feel is better, or it could be removed
> from the default output entirely which was what I did originally
> before submitting the patch.

Unless someone clamors for the added information,
let's just omit it from the default format strings.

I'll push these two change-sets shortly.

>From d6d47eedb938743a56431988e73a5a3205687ba9 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 5 Jul 2010 17:16:23 +0200
Subject: [PATCH 1/2] system.h: define ATTRIBUTE_WARN_UNUSED_RESULT

* src/system.h (ATTRIBUTE_WARN_UNUSED_RESULT): Define.
---
 src/system.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/src/system.h b/src/system.h
index 859b663..d3fd901 100644
--- a/src/system.h
+++ b/src/system.h
@@ -483,6 +483,14 @@ enum
 # define ATTRIBUTE_UNUSED __attribute__ ((__unused__))
 #endif

+/* The warn_unused_result attribute appeared first in gcc-3.4.0 */
+#undef ATTRIBUTE_WARN_UNUSED_RESULT
+#if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 4)
+# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
+#else
+# define ATTRIBUTE_WARN_UNUSED_RESULT /* empty */
+#endif
+
 #if defined strdupa
 # define ASSIGN_STRDUPA(DEST, S)               \
   do { DEST = strdupa (S); } while (0)
--
1.7.2.rc1.192.g262ff


>From e78f266f2ec5489824a28ef710c82bd6ca5ccf01 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 5 Jul 2010 17:18:29 +0200
Subject: [PATCH 2/2] stat: getfilecon failure now evokes nonzero exit status

Add comments and adjust interfaces to allow low-level failure
to propagate out to callers.
* src/stat.c (out_file_context): Return bool, not void,
so we can tell callers about failure.
(print_statfs, print_stat, print_it): Propagate failure to caller.
(do_statfs): Propagate print_it failure to caller.
(do_stat): Likewise.
I nearly forgot to update do_stat to propagate print_it failure,
and it compiled just fine in spite of that.  To prevent possibility
of a repeat, I've marked each function that returns non-void with
ATTRIBUTE_WARN_UNUSED_RESULT.
---
 src/stat.c |   60 ++++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/src/stat.c b/src/stat.c
index f1b5ef1..c3730f0 100644
--- a/src/stat.c
+++ b/src/stat.c
@@ -88,7 +88,7 @@
 /* BeOS has a statvfs function, but it does not return sensible values
    for f_files, f_ffree and f_favail, and lacks f_type, f_basetype and
    f_fstypename.  Use 'struct fs_info' instead.  */
-static int
+static int ATTRIBUTE_WARN_UNUSED_RESULT
 statfs (char const *filename, struct fs_info *buf)
 {
   dev_t device = dev_for_path (filename);
@@ -185,7 +185,7 @@ static char const *trailing_delim = "";
    Others have statfs.f_fstypename[MFSNAMELEN] (NetBSD 1.5.2).
    Still others have neither and have to get by with f_type (GNU/Linux).
    But f_type may only exist in statfs (Cygwin).  */
-static char const *
+static char const * ATTRIBUTE_WARN_UNUSED_RESULT
 human_fstype (STRUCT_STATVFS const *statfsbuf)
 {
 #ifdef STATXFS_FILE_SYSTEM_TYPE_MEMBER_NAME
@@ -436,7 +436,7 @@ human_fstype (STRUCT_STATVFS const *statfsbuf)
 #endif
 }

-static char *
+static char * ATTRIBUTE_WARN_UNUSED_RESULT
 human_access (struct stat const *statbuf)
 {
   static char modebuf[12];
@@ -445,7 +445,7 @@ human_access (struct stat const *statbuf)
   return modebuf;
 }

-static char *
+static char * ATTRIBUTE_WARN_UNUSED_RESULT
 human_time (struct timespec t)
 {
   static char str[MAX (INT_BUFSIZE_BOUND (intmax_t),
@@ -491,11 +491,14 @@ out_uint_x (char *pformat, size_t prefix_len, uintmax_t 
arg)
 }

 /* Very specialized function (modifies FORMAT), just so as to avoid
-   duplicating this code between both print_statfs and print_stat.  */
-static void
+   duplicating this code between both print_statfs and print_stat.
+   Return zero upon success, nonzero upon failure.  */
+static bool ATTRIBUTE_WARN_UNUSED_RESULT
 out_file_context (char const *filename, char *pformat, size_t prefix_len)
 {
   char *scontext;
+  bool fail = false;
+
   if ((follow_links
        ? getfilecon (filename, &scontext)
        : lgetfilecon (filename, &scontext)) < 0)
@@ -503,19 +506,22 @@ out_file_context (char const *filename, char *pformat, 
size_t prefix_len)
       error (0, errno, _("failed to get security context of %s"),
              quote (filename));
       scontext = NULL;
+      fail = true;
     }
   strcpy (pformat + prefix_len, "s");
   printf (pformat, (scontext ? scontext : "?"));
   if (scontext)
     freecon (scontext);
+  return fail;
 }

-/* print statfs info */
-static void
+/* Print statfs info.  Return zero upon success, nonzero upon failure.  */
+static bool ATTRIBUTE_WARN_UNUSED_RESULT
 print_statfs (char *pformat, size_t prefix_len, char m, char const *filename,
               void const *data)
 {
   STRUCT_STATVFS const *statfsbuf = data;
+  bool fail = false;

   switch (m)
     {
@@ -589,22 +595,24 @@ print_statfs (char *pformat, size_t prefix_len, char m, 
char const *filename,
       out_int (pformat, prefix_len, statfsbuf->f_ffree);
       break;
     case 'C':
-      out_file_context (filename, pformat, prefix_len);
+      fail |= out_file_context (filename, pformat, prefix_len);
       break;
     default:
       fputc ('?', stdout);
       break;
     }
+  return fail;
 }

-/* print stat info */
-static void
+/* Print stat info.  Return zero upon success, nonzero upon failure.  */
+static bool
 print_stat (char *pformat, size_t prefix_len, char m,
             char const *filename, void const *data)
 {
   struct stat *statbuf = (struct stat *) data;
   struct passwd *pw_ent;
   struct group *gw_ent;
+  bool fail = false;

   switch (m)
     {
@@ -620,7 +628,7 @@ print_stat (char *pformat, size_t prefix_len, char m,
             {
               error (0, errno, _("cannot read symbolic link %s"),
                      quote (filename));
-              return;
+              return true;
             }
           printf (" -> ");
           out_string (pformat, prefix_len, quote (linkname));
@@ -714,12 +722,13 @@ print_stat (char *pformat, size_t prefix_len, char m,
         out_uint (pformat, prefix_len, statbuf->st_ctime);
       break;
     case 'C':
-      out_file_context (filename, pformat, prefix_len);
+      fail |= out_file_context (filename, pformat, prefix_len);
       break;
     default:
       fputc ('?', stdout);
       break;
     }
+  return fail;
 }

 /* Output a single-character \ escape.  */
@@ -763,11 +772,16 @@ print_esc_char (char c)
   putchar (c);
 }

-static void
+/* Print the information specified by the format string, FORMAT,
+   calling PRINT_FUNC for each %-directive encountered.
+   Return zero upon success, nonzero upon failure.  */
+static bool ATTRIBUTE_WARN_UNUSED_RESULT
 print_it (char const *format, char const *filename,
-          void (*print_func) (char *, size_t, char, char const *, void const 
*),
+          bool (*print_func) (char *, size_t, char, char const *, void const 
*),
           void const *data)
 {
+  bool fail = false;
+
   /* Add 2 to accommodate our conversion of the stat `%s' format string
      to the longer printf `%llu' one.  */
   enum
@@ -807,7 +821,7 @@ print_it (char const *format, char const *filename,
                 putchar ('%');
                 break;
               default:
-                print_func (dest, len + 1, *fmt_char, filename, data);
+                fail |= print_func (dest, len + 1, *fmt_char, filename, data);
                 break;
               }
             break;
@@ -866,10 +880,12 @@ print_it (char const *format, char const *filename,
   free (dest);

   fputs (trailing_delim, stdout);
+
+  return fail;
 }

 /* Stat the file system and print what we find.  */
-static bool
+static bool ATTRIBUTE_WARN_UNUSED_RESULT
 do_statfs (char const *filename, bool terse, char const *format)
 {
   STRUCT_STATVFS statfsbuf;
@@ -899,12 +915,12 @@ do_statfs (char const *filename, bool terse, char const 
*format)
                 "Inodes: Total: %-10c Free: %d\n");
     }

-  print_it (format, filename, print_statfs, &statfsbuf);
-  return true;
+  bool fail = print_it (format, filename, print_statfs, &statfsbuf);
+  return ! fail;
 }

 /* stat the file and print what we find */
-static bool
+static bool ATTRIBUTE_WARN_UNUSED_RESULT
 do_stat (char const *filename, bool terse, char const *format)
 {
   struct stat statbuf;
@@ -959,8 +975,8 @@ do_stat (char const *filename, bool terse, char const 
*format)
             }
         }
     }
-  print_it (format, filename, print_stat, &statbuf);
-  return true;
+  bool fail = print_it (format, filename, print_stat, &statbuf);
+  return ! fail;
 }

 void
--
1.7.2.rc1.192.g262ff