Re: bug#10363: df vs. long-named /dev/disk/by-uuid/... file system device names

[Jim Meyering]

Paul Eggert wrote:
> On 12/29/11 06:09, Jim Meyering wrote:
>> +  /* If dev_name is a long-named symlink like
>> +     /dev/disk/by-uuid/828fc648-9f30-43d8-a0b1-f7196a2edb66 and its
>> +     canonical name is shorter, use the shorter name.  But don't bother
>> +     checking when DEV_NAME is no longer than e.g., "/dev/sda1"  */
>> +  if (resolve_device_symlink && 9 < orig_len
>> +      && (resolved_dev = canonicalize_filename_mode (dev_name, 
>> CAN_EXISTING)))
>> +    {
>> +      if (strlen (resolved_dev) < orig_len)
>> +        {
>> +          free (dev_name);
>> +          dev_name = resolved_dev;
>> +        }
>> +      else
>> +        {
>> +          free (resolved_dev);
>> +        }
>> +    }
>
> I have some qualms about that "is shorter" part;
> couldn't that lead to confusing results, on systems
> where the canonical name is sometimes a bit shorter and sometimes
> a bit longer?
>
> Also, that "9 < orig_len" could also cause confusion.
>
> The flag "resolve_device_symlink" suggests that
> the name should always be resolved, at any rate.
>
> In short, how a simpler approach, that always resolves
> symlinks?  Something like this:
>
>   /* If dev_name is a symlink use the resolved name.
>      On recent GNU/Linux systems we often see a symlink from, e.g.,
>      /dev/disk/by-uuid/828fc648-9f30-43d8-a0b1-f7196a2edb66
>      tp /dev/sda3 and it's better to output /dev/sda3.  */
>   if (resolve_device_symlink
>       && (resolved_dev = canonicalize_filename_mode (dev_name, CAN_EXISTING)))
>     {
>       free (dev_name);
>       dev_name = resolved_dev;
>    }

Thanks for the suggestion.  I've dropped the 9 < ... part, but since this
is (at least planned) to be the default, I want to limit the scope of the
change to those very long by-uuid symlinks, so I've adjusted it like this:

  /* On some systems, dev_name is a long-named symlink like
     /dev/disk/by-uuid/828fc648-9f30-43d8-a0b1-f7196a2edb66 pointing
     to a much shorter and more useful name like /dev/sda1.
     When resolve_device_symlink is true and dev_name is a symlink whose
     name starts with /dev/disk/by-uuid/ use the resolved name instead.  */
  if (resolve_device_symlink
      && STRPREFIX (dev_name, "/dev/disk/by-uuid/")
      && (resolved_dev = canonicalize_filename_mode (dev_name, CAN_EXISTING)))
    {
      free (dev_name);
      dev_name = resolved_dev;
    }

I considered matching only "/dev/disk/by-", in case some initrd
uses by-id, by-label or by-path, but I doubt that will happen often
enough, so will keep this change very precisely targeted.

I've also changed the parameter name from resolve_device_symlink to
process_all.  I don't particular like that name either.
Suggestions welcome.


>From fb52b3722433d88af8ca8912d3cac531e04bf585 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Thu, 29 Dec 2011 14:49:00 +0100
Subject: [PATCH] df: work around long-named /dev/disk/by-uuid/... symlinks

On systems with recent kernel/tools, a symlink from /etc/mtab to
/proc/mounts, and a by-UUID mount (i.e., soon, nearly everyone),
you will see something like the following when running "df -hT":
(this has been truncated to fit in a width-limited ChangeLog file)

Filesystem                                             Type      Siz...
rootfs                                                 rootfs     11G
udev                                                   devtmpfs  3.8G
tmpfs                                                  tmpfs     774M
/dev/disk/by-uuid/828fc648-9f30-43d8-a0b1-f7096a2edb66 ext4       11G
tmpfs                                                  tmpfs     1.6G
/dev/sda2                                              ext3      494M
/dev/sda5                                              ext4       12G
/dev/sda6                                              ext4      9.9G

Contrast that with what we're used to seeing (modulo the
two entries mounted on "/", which is a separate problem):

Filesystem     Type      Size  Used Avail Use% Mounted on
rootfs         rootfs     11G  1.9G  8.0G  19% /
udev           devtmpfs  3.8G     0  3.8G   0% /dev
tmpfs          tmpfs     774M  376K  774M   1% /run
/dev/sda3      ext4       11G  1.9G  8.0G  19% /
tmpfs          tmpfs     1.6G  8.0K  1.6G   1% /run/shm
/dev/sda2      ext3      494M   78M  392M  17% /boot
/dev/sda5      ext4       12G  7.6G  3.7G  68% /usr
/dev/sda6      ext4      9.9G  6.6G  2.8G  71% /var

When that long /dev/disk/by-uuid/... name is merely a symlink
to a much shorter (and often more useful) device name like
"/dev/sda3", and when it's part of a listing of all file systems,
I would much prefer to see only the latter.

I.e., if I explicitly run
"df -hT /dev/disk/by-uuid/828fc648-9f30-43d8-a0b1-f7096a2edb66",
then, it's fine -- and expected -- to print to the long name.
It was explicitly given.  However, with no non-option argument,
df should print the shorter name.  Note that performing this
translation at a lower level (via a change to gnulib's mountlist.c)
would make it impossible to distinguish those two cases.

* src/df.c: Include "canonicalize.h".
(get_dev): Add a parameter, telling when we're in process-all-
mount-points mode; update all callers.  When true, resolve
/dev/disk/by-uuid/... symlinks.
Reported by Dan Jacobson in http://bugs.gnu.org/10363
---
 src/df.c |   37 +++++++++++++++++++++++++++++--------
 1 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/src/df.c b/src/df.c
index 982d607..cedb583 100644
--- a/src/df.c
+++ b/src/df.c
@@ -25,6 +25,7 @@
 #include <assert.h>

 #include "system.h"
+#include "canonicalize.h"
 #include "error.h"
 #include "fsusage.h"
 #include "human.h"
@@ -428,13 +429,16 @@ add_uint_with_neg_flag (uintmax_t *dest, bool *dest_neg,
    If FSTYPE is non-NULL, it is the type of the file system on DISK.
    If MOUNT_POINT is non-NULL, then DISK may be NULL -- certain systems may
    not be able to produce statistics in this case.
-   ME_DUMMY and ME_REMOTE are the mount entry flags.  */
+   ME_DUMMY and ME_REMOTE are the mount entry flags.
+   Caller must set PROCESS_ALL to true when iterating over all entries, as
+   when df is invoked with no non-option argument.  See below for details.  */

 static void
 get_dev (char const *disk, char const *mount_point,
          char const *stat_file, char const *fstype,
          bool me_dummy, bool me_remote,
-         const struct fs_usage *force_fsu)
+         const struct fs_usage *force_fsu,
+         bool process_all)
 {
   struct fs_usage fsu;
   char buf[LONGEST_HUMAN_READABLE + 2];
@@ -488,6 +492,23 @@ get_dev (char const *disk, char const *mount_point,

   if (! disk)
     disk = "-";                        /* unknown */
+
+  char *dev_name = xstrdup (disk);
+  char *resolved_dev;
+
+  /* On some systems, dev_name is a long-named symlink like
+     /dev/disk/by-uuid/828fc648-9f30-43d8-a0b1-f7196a2edb66 pointing
+     to a much shorter and more useful name like /dev/sda1.
+     When process_all is true and dev_name is a symlink whose name starts
+     with /dev/disk/by-uuid/ use the resolved name instead.  */
+  if (process_all
+      && STRPREFIX (dev_name, "/dev/disk/by-uuid/")
+      && (resolved_dev = canonicalize_filename_mode (dev_name, CAN_EXISTING)))
+    {
+      free (dev_name);
+      dev_name = resolved_dev;
+    }
+
   if (! fstype)
     fstype = "-";              /* unknown */

@@ -537,7 +558,7 @@ get_dev (char const *disk, char const *mount_point,
       switch (field)
         {
         case DEV_FIELD:
-          cell = xstrdup (disk);
+          cell = dev_name;
           break;

         case TYPE_FIELD:
@@ -648,7 +669,7 @@ get_disk (char const *disk)
     {
       get_dev (best_match->me_devname, best_match->me_mountdir, NULL,
                best_match->me_type, best_match->me_dummy,
-               best_match->me_remote, NULL);
+               best_match->me_remote, NULL, false);
       return true;
     }

@@ -734,7 +755,7 @@ get_point (const char *point, const struct stat *statp)
   if (best_match)
     get_dev (best_match->me_devname, best_match->me_mountdir, point,
              best_match->me_type, best_match->me_dummy, best_match->me_remote,
-             NULL);
+             NULL, false);
   else
     {
       /* We couldn't find the mount entry corresponding to POINT.  Go ahead and
@@ -745,7 +766,7 @@ get_point (const char *point, const struct stat *statp)
       char *mp = find_mount_point (point, statp);
       if (mp)
         {
-          get_dev (NULL, mp, NULL, NULL, false, false, NULL);
+          get_dev (NULL, mp, NULL, NULL, false, false, NULL, false);
           free (mp);
         }
     }
@@ -774,7 +795,7 @@ get_all_entries (void)

   for (me = mount_list; me; me = me->me_next)
     get_dev (me->me_devname, me->me_mountdir, NULL, me->me_type,
-              me->me_dummy, me->me_remote, NULL);
+             me->me_dummy, me->me_remote, NULL, true);
 }

 /* Add FSTYPE to the list of file system types to display. */
@@ -1066,7 +1087,7 @@ main (int argc, char **argv)
     {
       if (inode_format)
         grand_fsu.fsu_blocks = 1;
-      get_dev ("total", NULL, NULL, NULL, false, false, &grand_fsu);
+      get_dev ("total", NULL, NULL, NULL, false, false, &grand_fsu, false);
     }

   print_table ();
--
1.7.7.3