[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: patching dd
From: |
Jim Meyering |
Subject: |
Re: patching dd |
Date: |
Sun, 09 Nov 2003 10:27:45 +0100 |
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;