Re: [PATCH 1/3] hexdump: more output formatting

[Satyam Sharma]

On 5/31/07, Randy Dunlap <randy.dunlap@xxxxxxxxxx> wrote:
> Satyam Sharma wrote:
> > Hello Randy,
> >
> >> Add a prefix string parameter.  Callers are responsible for any
> >> string length/alignment that they want to see in the output.  I.e.,
> >> callers should pad strings to achieve alignment if they want that.
> >>
> >> Add rowsize parameter.  This is the number of raw data bytes
> >> to be printed per line.  Must be 16 or 32.
> >>
> >> Add a group_size parameter.  This allows callers to dump values
> >> as 1-byte, 2-byte, 4-byte, or 8-byte numbers.  Default is
> >> 1-byte numbers.  If the total length is not an even multiple
> >> of group_size, 1-byte numbers are printed.
> >
> > I wonder if (over-)engineering could hurt its adoption by more kernel
> > users. There aren't very many 8-argument monsters in the kernel,
> > and one would expect hexdump() to be easier on the fingers to
> > type? :-) Why not just continue with reasonable default/fixed rowsize
> > and groupsize values?
>
> Yes, one can try to satisfy the users/callers by adapting to their
> needs (wishes), which requires parameters or such; or one can have
> no callers and end up with (around 11 currently) different hex dump
> functions in the kernel source tree, but no users of lib/hexdump.c.

Yes, you're right, but I was just wondering whether any users really
cared enough about the rowsize and groupsize, also seeing that
accommodating these two args leads to a lot of increase in code.

> But it won't take much more "requirements" for me to drop the patch.

Please, don't drop this! I only complained because when global or
commonly-used functions have very long arglists, one tends to forget
which arg goes at which number, and it becomes necessary to write
the calls with having the prototype simultaneously open in another
terminal for reference! ...

> >> Add an "ascii" output parameter.  This causes ASCII data output
> >> following the hex data output.
> > [...]
> >> +       if (!ascii)
> >> +               goto nil;
> >> +
> >> +       if ((lx + 1) < linebuflen)
> >>                 linebuf[lx++] = ' ';
> >> -       }
> >> -       for (j = 0; (j < 16) && (j < len) && (lx + 2) < linebuflen; j++)
> >> +       for (j = 0; (j < rowsize) && (j < len) && (lx + 2) <
> >> linebuflen; j++)
> >>                 linebuf[lx++] = isprint(ptr[j]) ? ptr[j] : '.';
> >> +nil:
> >>         linebuf[lx++] = '\0';
> >
> > if (ascii) {
> > ...
> > }
> >
> > linebuf[lx++] = '\0';
> >
> > would help lose a goto and label.
>
> The label has another reference already, so it wouldn't be lost.

Oh yes, I'd missed that if (!len) goto nil; -- sorry for the noise.

Thanks,
Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/