Re: df command should suppress duplicates

[Bernhard Voelker]

On 08/15/2012 05:41 PM, Ondrej Oprala wrote:
> Hi,
>   I've written a little fix for this bugzilla 
> https://bugzilla.redhat.com/show_bug.cgi?id=709351
> which I think could solve the duplicity output problem. The idea is that 
> df would keep a linked
> list of device numbers it already went through and not count in 
> (get_dev) those filesystems again
> when running into them as bind mounts.
> Cheers,
>   Ondrej.

Thanks for working on that.

> +/* Global device number linked list.  */
> +static struct devlist *devlist;
> +
> +/* Help pointer to devlist's first entry.  */
> +static struct devlist *devlist_head;
> +

I think we don't need both as static global variable.
The former is only used as a loop variable, so I'd define
it where it's needed.


> +/* Add a device number of the soon-to-be examined
> +   filesystem to the global list devlist.  */
> +
> +static void
> +devlist_add (dev_t dev_num)
> +{
> +  devlist->dev_num = dev_num;
> +  devlist->dev_next = xmalloc (sizeof *devlist);
> +  devlist = devlist->dev_next;
> +  devlist->dev_num = 0;
> +  devlist->dev_next = NULL;
> +  devlist = devlist_head;
> +}

Together with the new code in main, you allocate one item
more than what is needed. If the loop in dev_examinded()
would run "while (devlist)", then this wouldn't be necessary.

> +/* Check if the device was already examined.  */
> +
> +static bool
> +dev_examined (char *mount_dir)
> +{
> +  dev_t dev_num;
> +  struct stat *buf = alloca (sizeof *buf);

Isn't the use of alloca discouraged? A simple "struct stat buf"
would be sufficient.

> +  stat (mount_dir, buf);
> +
> +  dev_num = buf->st_dev;
> +  while (devlist->dev_num)
> +      {
> +        if (devlist->dev_num == dev_num)
> +          {
> +            devlist = devlist_head;
> +            return true;
> +          }
> +        devlist = devlist->dev_next;
> +      }
> +
> +  devlist_add (dev_num);
> +  return false;
> +}
...
> @@ -830,8 +882,11 @@ get_all_entries (void)
>    struct mount_entry *me;
>  
>    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, true);
> +    if (!dev_examined (me->me_mountdir))
> +      {
> +      get_dev (me->me_devname, me->me_mountdir, NULL, me->me_type,
> +               me->me_dummy, me->me_remote, NULL, true);
> +      }
>  }
...
> @@ -1132,7 +1187,21 @@ main (int argc, char **argv)
>            get_entry (argv[i], &stats[i - optind]);
>      }
>    else
> -    get_all_entries ();
> +    {
> +      devlist = xmalloc (sizeof *devlist);
> +      devlist->dev_num = 0;
> +      devlist->dev_next = NULL;
> +      devlist_head = devlist;

See my comment above: allocating one more than actually needed.

> +
> +      get_all_entries ();
> +
> +      while (devlist_head)
> +        {
> +          devlist = devlist_head->dev_next;
> +          free (devlist_head);
> +          devlist_head = devlist;
> +        }
> +    }
>  
>    if (print_grand_total && file_systems_processed)
>      {

There's no test, although I personally think it's hard to
write a test ... well, maybe by faking /etc/mtab, i.e. by
intercepting getmntent() in another LD_PRELOAD'ed test? ;-)

The practical result is good, although - as a user - I'd expect
that the "rootfs" line would disappear instead of the line showing
the device which is mounted on '/'. E.g. on my PC, I now see

  rootfs           12G  7.2G  3.9G  66% /

instead of

  /dev/sda1        12G  7.2G  3.9G  66% /

Even the new findmnt command of util-linux shows the device:

  $ ~/ul/findmnt --df
  SOURCE         FSTYPE                SIZE   USED AVAIL USE% TARGET
  ...
  /dev/sda1      ext4                 11.5G   7.1G  4.4G  62% /
  ...


Have a nice day,
Berny