Re: [ndctl PATCH v4 09/17] util/hexdump: Add a util helper to print a buffer in hex

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Thu, 2021-10-14 at 09:48 -0700, Dan Williams wrote:
> On Thu, Oct 7, 2021 at 1:22 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > 
> > In preparation for tests that may need to set, retrieve, and display
> > opaque data, add a hexdump function in util/
> > 
> > Cc: Ben Widawsky <ben.widawsky@intel.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  util/hexdump.h |  8 ++++++++
> >  util/hexdump.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> If this is just for tests shouldn't it go in tests/ with the other
> common test helpers? If it stays in util/ I would kind of expect it to
> use the log infrastructure, no?

Agreed on using the log infra. I was originally using it in the old
test stuff, but right now there's no user for it.. However having
something like this was nice when developing the early cmd submission
stuff. Do you think it would be good to always do a hexdump with dbg()
when under --verbose for evert cxl_cmd_submit? (and maybe even add it
for ndctl_cmd_submit later too) Or is that too noisy?

If we want to do that then it makes sense to redo with the logging api,
else maybe jsut drop this until we have a real in-tree user?

> 
> Other than that looks ok to me:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> 
> >  2 files changed, 61 insertions(+)
> >  create mode 100644 util/hexdump.h
> >  create mode 100644 util/hexdump.c
> > 
> > diff --git a/util/hexdump.h b/util/hexdump.h
> > new file mode 100644
> > index 0000000..d322b6a
> > --- /dev/null
> > +++ b/util/hexdump.h
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (C) 2021 Intel Corporation. All rights reserved. */
> > +#ifndef _UTIL_HEXDUMP_H_
> > +#define _UTIL_HEXDUMP_H_
> > +
> > +void hex_dump_buf(unsigned char *buf, int size);
> > +
> > +#endif /* _UTIL_HEXDUMP_H_*/
> > diff --git a/util/hexdump.c b/util/hexdump.c
> > new file mode 100644
> > index 0000000..1ab0118
> > --- /dev/null
> > +++ b/util/hexdump.c
> > @@ -0,0 +1,53 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (C) 2015-2021 Intel Corporation. All rights reserved. */
> > +#include <stdio.h>
> > +#include <util/hexdump.h>
> > +
> > +static void print_separator(int len)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < len; i++)
> > +               fprintf(stderr, "-");
> > +       fprintf(stderr, "\n");
> > +}
> > +
> > +void hex_dump_buf(unsigned char *buf, int size)
> > +{
> > +       int i;
> > +       const int grp = 4;  /* Number of bytes in a group */
> > +       const int wid = 16; /* Bytes per line. Should be a multiple of grp */
> > +       char ascii[wid + 1];
> > +
> > +       /* Generate header */
> > +       print_separator((wid * 4) + (wid / grp) + 12);
> > +
> > +       fprintf(stderr, "Offset    ");
> > +       for (i = 0; i < wid; i++) {
> > +               if (i % grp == 0) fprintf(stderr, " ");
> > +               fprintf(stderr, "%02x ", i);
> > +       }
> > +       fprintf(stderr, "  Ascii\n");
> > +
> > +       print_separator((wid * 4) + (wid / grp) + 12);
> > +
> > +       /* Generate hex dump */
> > +       for (i = 0; i < size; i++) {
> > +               if (i % wid == 0) fprintf(stderr, "%08x  ", i);
> > +               ascii[i % wid] =
> > +                   ((buf[i] >= ' ') && (buf[i] <= '~')) ? buf[i] : '.';
> > +               if (i % grp == 0) fprintf(stderr, " ");
> > +               fprintf(stderr, "%02x ", buf[i]);
> > +               if ((i == size - 1) && (size % wid != 0)) {
> > +                       int j;
> > +                       int done = size % wid;
> > +                       int grps_done = (done / grp) + ((done % grp) ? 1 : 0);
> > +                       int spaces = wid / grp - grps_done + ((wid - done) * 3);
> > +
> > +                       for (j = 0; j < spaces; j++) fprintf(stderr, " ");
> > +               }
> > +               if ((i % wid == wid - 1) || (i == size - 1))
> > +                       fprintf(stderr, "  %.*s\n", (i % wid) + 1, ascii);
> > +       }
> > +       print_separator((wid * 4) + (wid / grp) + 12);
> > +}
> > --
> > 2.31.1
> > 
>