| Message ID | 20211007082139.3088615-10-vishal.l.verma@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Initial CXL support | expand |
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? 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 >
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 > > >
On Thu, Oct 14, 2021 at 1:33 PM Verma, Vishal L <vishal.l.verma@intel.com> wrote: > > 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? It sounds good as an extra-verbose debug option. At least it would be more personal preference that -v does not get any more noisy by default and require -vv to get hexdumps. > 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? That's always ok in my book.
On Thu, 2021-10-14 at 15:39 -0700, Dan Williams wrote: > On Thu, Oct 14, 2021 at 1:33 PM Verma, Vishal L > <vishal.l.verma@intel.com> wrote: > > > > 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? > > It sounds good as an extra-verbose debug option. At least it would be > more personal preference that -v does not get any more noisy by > default and require -vv to get hexdumps. > > > 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? > > That's always ok in my book. > I'll drop it for now. If we ever need it, lore has it archived forever :)
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); +}
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 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100644 util/hexdump.h create mode 100644 util/hexdump.c