Package: coreutils;
Reported by: A Burgie <dajoker <at> gmail.com>
Date: Fri, 2 Jul 2010 20:55:01 UTC
Severity: normal
Done: Pádraig Brady <P <at> draigBrady.com>
Bug is archived. No further changes may be made.
Message #23 received at 6555 <at> debbugs.gnu.org (full text, mbox):
From: Jim Meyering <jim <at> meyering.net> To: A Burgie <dajoker <at> gmail.com> Cc: 6555 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com> Subject: Re: bug#6555: stat enhancement Date: Mon, 05 Jul 2010 17:26:43 +0200
A Burgie wrote:
> On Mon, Jul 5, 2010 at 07:50, Jim Meyering <jim <at> meyering.net> wrote:
>> A Burgie wrote:
>>> On Mon, Jul 5, 2010 at 06:48, Jim Meyering <jim <at> meyering.net> 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 <meyering <at> redhat.com>
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 <meyering <at> redhat.com>
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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.