Re: TODO: --total option to df

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Gustavo Rondina on 8/7/2006 8:11 PM:
> Hello,
> 
> 
> I was looking the coreutils TODO list and saw that there was still a
> request
> for a --total option to df. I have written a patch to implement this
> feature. It is attached to this message. It worked for me, but I'm no sure
> if it is fully functional and bug free. This is my first "serious" patch.
> Any suggestions are welcome.

Thanks for the contribution.  However, it is not trivial, so to be
incorporated, you will need to assign copyright to the FSF.  If you are
willing, we can get that started.  Can you convince your mailer to attach
patches with a text MIME type?  It makes reviewing a lot easier.

I'm not the maintainer, so I can't say whether your patch will be
accepted, even if you do complete copyright paperwork.  But I did notice
some problems with just a brief glance.

> --- coreutils-5.97.orig/src/df.c      2006-08-07 22:40:01.000000000 -0300
> +++ coreutils-5.97/src/df.c   2006-08-07 22:30:04.000000000 -0300

Please make your patch against CVS head.  5.97 is a stable branch, and new
features are only being added to head at the moment.

> @@ -36,6 +36,7 @@
>  #include "quote.h"
>  #include "save-cwd.h"
>  #include "xgetcwd.h"
> +#include "xstrtol.h"
>
>  /* The official name of this program (e.g., no `g' prefix).  */
>  #define PROGRAM_NAME "df"
> @@ -60,12 +61,27 @@
>     command line argument -- even if it's a dummy (automounter) entry.  */
>  static bool show_listed_fs;
>
> +/* If true, show summary on total usage and availability. */
> +static bool show_total;
> +
>  /* Human-readable options for output.  */
>  static int human_output_opts;
>
>  /* The units to use when printing sizes.  */
>  static uintmax_t output_block_size;
>
> +/* Number of blocks. */
> +struct storage_size
> +{
> +  uintmax_t n;
> +  bool negative;
> +};
> +
> +/* Total number of blocks. */
> +static struct storage_size total_blocks;
> +static struct storage_size total_used;
> +static struct storage_size total_available;
> +
>  /* If true, use the POSIX output format.  */
>  static bool posix_format;
>
> @@ -124,6 +140,7 @@
>  {
>    {"all", no_argument, NULL, 'a'},
>    {"block-size", required_argument, NULL, 'B'},
> +  {"total", no_argument, NULL, 'c'},
>    {"inodes", no_argument, NULL, 'i'},
>    {"human-readable", no_argument, NULL, 'h'},
>    {"si", no_argument, NULL, 'H'},
> @@ -252,6 +269,38 @@
>      }
>  }
>
> +/* Add N to the total amount of DEST, which can be total blocks,
> +   used blocks or available blocks. */
> +
> +static void
> +add_to_total (struct storage_size *dest, uintmax_t n,
> +           uintmax_t input_units, bool negate, char *buf)
> +{
> +  uintmax_t block_amount;
> +
> +  if (1024 != input_units)
> +    {
> +      int opts_backup = human_output_opts;
> +      char convert_buf[LONGEST_HUMAN_READABLE + 2];
> +      strtol_error s_err;
> +
> +      human_output_opts = 0;
> +      sprintf (convert_buf, "%s", df_readable (false, (negate ? -n : n),
> +                                               buf, input_units, 1024));
> +
> +      s_err = xstrtoumax (convert_buf, NULL, 10, &block_amount, "");
> +      if (s_err != LONGINT_OK)
> +        _STRTOL_ERROR (EXIT_FAILURE, convert_buf, _("size"), s_err);
> +
> +      human_output_opts = opts_backup;
> +    }
> +
> +  if (negate && (block_amount > dest->n))
> +    dest->negative = negate;
> +
> +  dest->n += (negate ? -block_amount : block_amount);
> +}
> +
>  /* Display a space listing for the disk device with absolute file name
DISK.
>     If MOUNT_POINT is non-NULL, it is the name of the root of the
>     file system on DISK.
> @@ -431,6 +480,13 @@
>        printf (" %s", mount_point);
>      }
>    putchar ('\n');
> +
> +  /* Sum this device's number of blocks to total. */
> +
> +  add_to_total (&total_blocks, total, input_units, false, buf[0]);
> +  add_to_total (&total_used, used, input_units, negate_used, buf[1]);
> +  add_to_total (&total_available, available, input_units,
> +             negate_available, buf[2]);
>  }
>
>  /* Return the root mountpoint of the file system on which FILE exists, in
> @@ -684,6 +740,90 @@
>             me->me_dummy, me->me_remote);
>  }
>
> +/* Show an one line summary on total usage and availabilitiy of
> +   selected devices. */
> +
> +static void
> +show_summary (void)
> +{
> +  char buf[3][LONGEST_HUMAN_READABLE + 2];
> +  int width;
> +  int use_width;
> +  double pct = -1;
> +
> +  printf("%-20s", "(total)");
> +
> +  if (inode_format)
> +    {
> +      width = 7;
> +      use_width = 5;
> +    }
> +  else
> +    {
> +      width = (human_output_opts & human_autoscale
> +               ? 5 + ! (human_output_opts & human_base_1024)
> +               : 9);
> +      use_width = ((posix_format
> +                    && ! (human_output_opts & human_autoscale))
> +                    ? 8 : 4);
> +    }
> +
> +  printf(" %*s %*s %*s ",
> +         width, df_readable (total_blocks.negative, total_blocks.n, buf[0],
> +                             1024, output_block_size),
> +         width, df_readable (total_used.negative, total_used.n, buf[1],
> +                             1024, output_block_size),
> +         width, df_readable (total_available.negative, total_available.n,
> +                             buf[2], 1024, output_block_size));
> +
> +  if (total_used.n == UINTMAX_MAX || total_available.n == UINTMAX_MAX)
> +    ;
> +  else if (!total_used.negative
> +        && total_used.n <= TYPE_MAXIMUM (uintmax_t) / 100
> +        && total_used.n + total_available.n != 0
> +        && (total_used.n + total_available.n < total_used.n)
> +              == total_available.negative)

Write those last two lines as:
           && ((total_used.n + total_available.n < total_used.n)
               == total_available.negative))
And be careful with indentation (the style is to use TAB in preference to
8 spaces outside of strings printed to the terminal).

> +    {
> +      uintmax_t u100 = total_used.n * 100;
> +      uintmax_t nonroot_total = total_used.n + total_available.n;
> +      pct = u100 / nonroot_total + (u100 % nonroot_total != 0);
> +    }
> +  else
> +    {
> +      /* The calculation cannot be done easily with integer
> +      arithmetic.  Fall back on floating point.  This can suffer
> +      from minor rounding errors, but doing it exactly requires
> +      multiple precision arithmetic, and it's not worth the
> +      aggravation.  */

I'm not sure this is the right approach.  Do we really expect systems with
more than 2^64 bytes of storage?

> +      double u = total_used.negative
> +                 ? - (double) - total_used.n
> +                 : total_used.n;

GNU Coding standards recommend () here:
      double u = (total_used.negative
                  ? - (double) - total_used.n
                  : total_used.n);

> +
> +      double a = total_available.negative
> +                 ? - (double) - total_available.n
> +                 : total_available.n;
> +
> +      double nonroot_total = u + a;
> +      if (nonroot_total)
> +     {
> +       long int lipct = pct = u * 100 / nonroot_total;
> +       double ipct = lipct;
> +
> +       /* Like `pct = ceil (dpct);', but avoid ceil so that
> +          the math library needn't be linked.  */

Why not?  We already link it when using a replacement pow(), and the
library's version is possibly more efficient.  Not to mention that I'm
still not convinced you need to use floating point.

> +       if (ipct - 1 < pct && pct <= ipct + 1)
> +         pct = ipct + (ipct < pct);
> +     }
> +    }
> +
> +  if (0 <= pct)
> +    printf ("%*.0f%%", use_width - 1, pct);
> +  else
> +    printf ("%*s", use_width, "- ");
> +
> +  putchar('\n');
> +}
> +
>  /* Add FSTYPE to the list of file system types to display. */
>
>  static void
> @@ -730,6 +870,7 @@
>        fputs (_("\
>    -a, --all             include dummy file systems\n\
>    -B, --block-size=SIZE use SIZE-byte blocks\n\
> +  -c, --total           print a summary on total usage and availability\n\
>    -h, --human-readable  print sizes in human readable format (e.g., 1K
234M 2G)\n\
>    -H, --si              likewise, but use powers of 1000 not 1024\n\
>  "), stdout);
> @@ -786,7 +927,7 @@
>    posix_format = false;
>    exit_status = EXIT_SUCCESS;
>
> -  while ((c = getopt_long (argc, argv, "aB:iF:hHklmPTt:vx:",
long_options, NULL))
> +  while ((c = getopt_long (argc, argv, "aB:ciF:hHklmPTt:vx:",
long_options, NULL))
>        != -1)
>      {
>        switch (c)
> @@ -797,6 +938,9 @@
>       case 'B':
>         human_output_opts = human_options (optarg, true, &output_block_size);
>         break;
> +        case 'c':
> +          show_total = true;
> +          break;

Again, indentation.

>       case 'i':
>         inode_format = true;
>         break;
> @@ -942,5 +1086,8 @@
>        show_all_entries ();
>      }
>
> +  if (show_total)
> +    show_summary ();
> +
>    exit (exit_status);
>  }

Your patch is missing changes to NEWS, TODO, and doc/coreutils.texi, as
well as a ChangeLog entry.

- --
Life is short - so eat dessert first!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFE2ItL84KuGfSFAYARAtZNAJ9PVa/mHAIqKJPZYMmtCIAQUK7ysgCggxLT
mPqP0UiUoTFbN1wGML+3RBU=
=CR53
-----END PGP SIGNATURE-----