Re: patching dd

[Jim Meyering]

Olivier Delhomme <address@hidden> wrote:
>

[25 lines of unnecessary context removed -- please trim context of
 quoted messages]

> Thank you for replying so quickly,
> sorry for that mistake, i'm so confused. Here is the patch
> against today's cvs sources :

Thank you for the patch.

Have you read the GNU Coding Standards document?
Run `info standards' on a Linux system.  There are
some minor syntactic problems below.  Most would be eliminated
if you used GNU indent output as a model.  If you use emacs,
using cc-mode helps for indentation.

This change is large enough that you'll have to send
a copyright assignment to the FSF.  Here are the instructions:

copyright-assignment-procedure
Description: Text document

Following are a lot of comments.
I've included most of those suggested changes in an alternative patch, below.

Note that I would like `display=human' to work like --human
on other tools (without requiring that dbs=N be specified),
so I added a little code.  This makes it so dbs=N specified
before `display=human' is *ignored*.  But that is at least consistent
with du:  The -B1 is ignored here:

  $ du -B1 -h -s dd.c
  40K     dd.c

but honored here:

  $ du -h -B1 -s dd.c
  40960   dd.c

Jim

> --- coreutils/src/dd.c        2003-11-05 23:11:31.000000000 +0100
> +++ coreutils-m/src/dd.c      2003-11-08 21:26:53.000000000 +0100
> @@ -35,6 +35,7 @@
>  #include "quote.h"
>  #include "safe-read.h"
>  #include "xstrtol.h"

Our convention is to alphabetize these #include lines.

...
> -print_stats (void)
> +print_human_stats(void)

There should be a space before the `('.

>  {
> -  char buf[2][INT_BUFSIZE_BOUND (uintmax_t)];
> +  char display_buf[5][MAX (LONGEST_HUMAN_READABLE + 1, INT_BUFSIZE_BOUND 
> (uintmax_t))];

No line should be longer than 80 bytes.

> +
>    fprintf (stderr, _("%s+%s records in\n"),

Isn't this printing the number of *bytes*, now,
rather than records?

> -        umaxtostr (r_full, buf[0]), umaxtostr (r_partial, buf[1]));
> +        human_readable (r_full, display_buf[0], human_output_opts, 
> input_blocksize, display_block_size), 

Likewise.

> +        human_readable (r_partial, display_buf[1], human_output_opts, 
> input_blocksize, display_block_size));
> +     
>    fprintf (stderr, _("%s+%s records out\n"),
> -        umaxtostr (w_full, buf[0]), umaxtostr (w_partial, buf[1]));
> +        human_readable (w_full, display_buf[2],  human_output_opts, 
> output_blocksize , display_block_size), 
> +        human_readable (w_partial, display_buf[3], human_output_opts, 
> output_blocksize, display_block_size));
> +     
>    if (r_truncate > 0)
>      {
>        fprintf (stderr, "%s %s\n",
> -            umaxtostr (r_truncate, buf[0]),
> -            (r_truncate == 1
> -             ? _("truncated record")
> +            human_readable (r_truncate, display_buf[5], human_output_opts, 
> output_blocksize, display_block_size),

Whoops!  The above [5] should be [4].

> +            (r_truncate == 1                                                 
>                                                         ? _("truncated 
> record")
Yow.  That's a very long line.

...
> +static void
> +print_stats (void)
> +{
> +  char buf[2][INT_BUFSIZE_BOUND (uintmax_t)];

The above declaration can be moved `down' into the scope where it's used.

> +  
> +  if (STREQ (display,"human"))

always include a space after `,'.

> +    { /* human readable format */
> +     print_human_stats();
> +    }
> +  else if (!STREQ (display,"quiet"))

Likewise.

> +   { /* in case dd should not be quiet */      <<<

Personally, I require that there be no trailing blanks.

> +     fprintf (stderr, _("%s+%s records in\n"),
> +                  umaxtostr (r_full, buf[0]), umaxtostr (r_partial, buf[1]));
> +     fprintf (stderr, _("%s+%s records out\n"),
> +          umaxtostr (w_full, buf[0]), umaxtostr (w_partial, buf[1]));
> +     if (r_truncate > 0)
> +       {
> +           fprintf (stderr, "%s %s\n",
> +                umaxtostr (r_truncate, buf[0]),
> +                (r_truncate == 1
> +                ?_("truncated record")

Nearly all operators should be followed by a space,
so there should be a space after `?'.

...
> +      else if (STREQ (name, "display")) // choose your display mode (quiet, 
> human, normal)

Don't use `//'.  That's for C++.
This is C.

> +     display = val;

You'll need to add code to diagnose an invalid value,
e.g. `display=wrong'.

...
> +  human_output_opts = human_options (getenv ("DD_DISPLAY_BLOCK_SIZE"), false,
> +                                                  &display_block_size); 

The continued line should be aligned with the opening `('.

>    /* Don't close stdout on exit from here on.  */
>    closeout_func = NULL;
>  

And, of course, any time someone adds a new feature,
it makes it easier for me if they also include a patch
for the texinfo documentation.

Here's a patch with most of the above changes:
[I've checked it in on the display_human branch of dd.c,
pending receipt by FSF of the necessary copyright paperwork]

Index: dd.c
===================================================================
RCS file: /fetish/cu/src/dd.c,v
retrieving revision 1.152
diff -u -p -r1.152 dd.c
--- dd.c        5 Nov 2003 03:53:19 -0000       1.152
+++ dd.c        9 Nov 2003 09:17:03 -0000
@@ -30,6 +30,7 @@
 #include "error.h"
 #include "full-write.h"
 #include "getpagesize.h"
+#include "human.h"
 #include "inttostr.h"
 #include "long-options.h"
 #include "quote.h"
@@ -143,6 +144,39 @@ static size_t oc = 0;
 /* Index into current line, for `conv=block' and `conv=unblock'.  */
 static size_t col = 0;
 
+enum Display_mode
+{
+  /* Generate the default, standards-conforming, style of output:
+     $ dd if=/dev/zero of=/dev/null count=10
+     10+0 records in
+     10+0 records out  */
+  DISP_DEFAULT,
+
+  /* Generate no such output.
+     $ dd if=/dev/zero of=/dev/null count=10 display=quiet
+     $ */
+  DISP_QUIET,
+
+  /* Generate more readable output:
+       $ dd if=/dev/zero of=/dev/null count=10 display=human
+       5.0K+0 bytes in
+       5.0K+0 bytes out
+     You can also specify the display block size:
+       $ dd if=/dev/zero of=/dev/null count=10 display=human dbs=1
+       5120+0 bytes in
+       5120+0 bytes out */
+  DISP_HUMAN_READABLE
+};
+
+/* This is aimed to choose diplay type */
+static enum Display_mode display_mode = DISP_DEFAULT;
+
+/* Human-readable options for output.  */
+static int human_output_opts;
+
+/* The units to use when printing sizes.  */
+static uintmax_t display_block_size;
+
 struct conversion
 {
   char *convname;
@@ -300,11 +334,24 @@ Copy a file, converting and formatting a
   of=FILE         write to FILE instead of stdout\n\
   seek=BLOCKS     skip BLOCKS obs-sized blocks at start of output\n\
   skip=BLOCKS     skip BLOCKS ibs-sized blocks at start of input\n\
+  display=MODE    uses display mode according to MODE\n\
+  dbs=SIZE        uses SIZE-byte blocks to display statistics\n\
 "), stdout);
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
       fputs (_("\
 \n\
+MODE may be:\n\
+  quiet          do not display statistics\n\
+  human          display statistics in a human readable format\n\
+\n\
+SIZE may be:\n\
+  human          prints all sizes in human readable format (e.g. 1K, 234M)\n\
+  si             likewise, but uses powers of 1000 instead of 1024\n\
+  BYTES          likewise, but use powers of BYTES\n\
+"),stdout);
+      fputs (_("\
+\n\
 BLOCKS and BYTES may be followed by the following multiplicative suffixes:\n\
 xM M, c 1, w 2, b 512, kB 1000, K 1024, MB 1000*1000, M 1024*1024,\n\
 GB 1000*1000*1000, G 1024*1024*1024, and so on for T, P, E, Z, Y.\n\
@@ -366,17 +413,29 @@ bit_count (register int i)
 }
 
 static void
-print_stats (void)
+print_human_stats (void)
 {
-  char buf[2][INT_BUFSIZE_BOUND (uintmax_t)];
-  fprintf (stderr, _("%s+%s records in\n"),
-          umaxtostr (r_full, buf[0]), umaxtostr (r_partial, buf[1]));
-  fprintf (stderr, _("%s+%s records out\n"),
-          umaxtostr (w_full, buf[0]), umaxtostr (w_partial, buf[1]));
-  if (r_truncate > 0)
+  char display_buf[5][MAX (LONGEST_HUMAN_READABLE + 1,
+                          INT_BUFSIZE_BOUND (uintmax_t))];
+
+  fprintf (stderr, _("%s+%s bytes in\n"),
+          human_readable (r_full, display_buf[0], human_output_opts,
+                          input_blocksize, display_block_size),
+          human_readable (r_partial, display_buf[1], human_output_opts,
+                          input_blocksize, display_block_size));
+
+  fprintf (stderr, _("%s+%s bytes out\n"),
+          human_readable (w_full, display_buf[2],  human_output_opts,
+                          output_blocksize , display_block_size),
+          human_readable (w_partial, display_buf[3], human_output_opts,
+                          output_blocksize, display_block_size));
+
+  if (0 < r_truncate)
     {
       fprintf (stderr, "%s %s\n",
-              umaxtostr (r_truncate, buf[0]),
+              human_readable (r_truncate, display_buf[4],
+                              human_output_opts, output_blocksize,
+                              display_block_size),
               (r_truncate == 1
                ? _("truncated record")
                : _("truncated records")));
@@ -384,6 +443,31 @@ print_stats (void)
 }
 
 static void
+print_stats (void)
+{
+  if (display_mode == DISP_HUMAN_READABLE)
+    {
+      print_human_stats ();
+    }
+  else if (display_mode == DISP_DEFAULT)
+    {
+      char buf[2][INT_BUFSIZE_BOUND (uintmax_t)];
+      fprintf (stderr, _("%s+%s records in\n"),
+              umaxtostr (r_full, buf[0]), umaxtostr (r_partial, buf[1]));
+      fprintf (stderr, _("%s+%s records out\n"),
+              umaxtostr (w_full, buf[0]), umaxtostr (w_partial, buf[1]));
+      if (r_truncate > 0)
+       {
+         fprintf (stderr, "%s %s\n",
+                  umaxtostr (r_truncate, buf[0]),
+                  (r_truncate == 1
+                   ? _("truncated record")
+                   : _("truncated records")));
+       }
+    }
+}
+
+static void
 cleanup (void)
 {
   print_stats ();
@@ -575,6 +659,26 @@ scanargs (int argc, char **argv)
        output_file = val;
       else if (STREQ (name, "conv"))
        parse_conversion (val);
+      else if (STREQ (name, "display")) /* select display mode */
+       {
+         if (STREQ (val, "human"))
+           {
+             display_mode = DISP_HUMAN_READABLE;
+             human_output_opts = human_autoscale | human_SI | human_base_1024;
+             display_block_size = 1;
+           }
+         else if (STREQ (val, "quiet"))
+           {
+             display_mode = DISP_QUIET;
+           }
+         else
+           {
+             error (0, 0, _("unrecognized display= argument %s"), quote (val));
+             usage (EXIT_FAILURE);
+           }
+       }
+      else if (STREQ (name, "dbs")) /* select display block size */
+       human_output_opts = human_options (val, true, &display_block_size);
       else
        {
          int invalid = 0;
@@ -1163,6 +1267,9 @@ main (int argc, char **argv)
   parse_long_options (argc, argv, PROGRAM_NAME, PACKAGE, VERSION,
                      usage, AUTHORS, (char const *) NULL);
 
+  human_output_opts = human_options (getenv ("DD_DISPLAY_BLOCK_SIZE"),
+                                    false, &display_block_size);
+
   /* Don't close stdout on exit from here on.  */
   closeout_func = NULL;