| Message ID | 20190410080151.429876-5-amitay@ozlabs.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hexdump bytes from getmem | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (854c4c5facff43af9e0fe5d7062b58f631987b0b) |
| snowpatch_ozlabs/build-multiarch | success | Test build-multiarch on branch master |
On Wed, 10 Apr 2019 at 08:03, Amitay Isaacs <amitay@ozlabs.org> wrote: > > If getmem is being run on a terminal and if the output contains > non-printable characters, then use hexdump() to dump the data. > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> Reviewed-by: Joel Stanley <joel@jms.id.au>
On Wednesday, 10 April 2019 6:01:50 PM AEST Amitay Isaacs wrote: > If getmem is being run on a terminal and if the output contains > non-printable characters, then use hexdump() to dump the data. To be honest really don't like this behavior being dependent on where stdout is going to. It seems like it would be suprising. For example if someone piped this into grep they would expect it to be running against the hexdump not the raw binary which could be confusing. The printable character detection is a great idea though. Can we just add a flag to force raw binary output instead of doing it based on write location? - Alistair > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > --- > src/mem.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/src/mem.c b/src/mem.c > index 8045069..b264d98 100644 > --- a/src/mem.c > +++ b/src/mem.c > @@ -21,6 +21,7 @@ > #include <unistd.h> > #include <assert.h> > #include <stdbool.h> > +#include <ctype.h> > > #include <libpdbg.h> > > @@ -28,6 +29,7 @@ > #include "progress.h" > #include "optcmd.h" > #include "parsers.h" > +#include "util.h" > > #define PR_ERROR(x, args...) \ > pdbg_log(PDBG_ERROR, x, ##args) > @@ -75,8 +77,22 @@ static int _getmem(uint64_t addr, uint64_t size, uint8_t > block_size, bool ci) } > > if (count > 0) { > - if (write(STDOUT_FILENO, buf, size) < 0) > - PR_ERROR("Unable to write stdout.\n"); > + uint64_t i; > + bool printable = true; > + > + for (i=0; i<size; i++) { > + if (!isprint(buf[i])) { > + printable = false; > + break; > + } > + } > + > + if (isatty(STDOUT_FILENO) && !printable) { > + hexdump(addr, buf, size, 1); > + } else { > + if (write(STDOUT_FILENO, buf, size) < 0) > + PR_ERROR("Unable to write stdout.\n"); > + } > } > > free(buf);
On Wed, 2019-04-17 at 15:20 +1000, Alistair Popple wrote: > On Wednesday, 10 April 2019 6:01:50 PM AEST Amitay Isaacs wrote: > > If getmem is being run on a terminal and if the output contains > > non-printable characters, then use hexdump() to dump the data. > > To be honest really don't like this behavior being dependent on where > stdout > is going to. It seems like it would be suprising. For example if > someone piped > this into grep they would expect it to be running against the hexdump > not the > raw binary which could be confusing. Fair enough. So by default it will use hexdump() unless the buffer is printable. And when given the extra flag, it'll be always binary. > > The printable character detection is a great idea though. Can we just > add a > flag to force raw binary output instead of doing it based on write > location? > > - Alistair > > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > > --- > > src/mem.c | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/src/mem.c b/src/mem.c > > index 8045069..b264d98 100644 > > --- a/src/mem.c > > +++ b/src/mem.c > > @@ -21,6 +21,7 @@ > > #include <unistd.h> > > #include <assert.h> > > #include <stdbool.h> > > +#include <ctype.h> > > > > #include <libpdbg.h> > > > > @@ -28,6 +29,7 @@ > > #include "progress.h" > > #include "optcmd.h" > > #include "parsers.h" > > +#include "util.h" > > > > #define PR_ERROR(x, args...) \ > > pdbg_log(PDBG_ERROR, x, ##args) > > @@ -75,8 +77,22 @@ static int _getmem(uint64_t addr, uint64_t size, > > uint8_t > > block_size, bool ci) } > > > > if (count > 0) { > > - if (write(STDOUT_FILENO, buf, size) < 0) > > - PR_ERROR("Unable to write stdout.\n"); > > + uint64_t i; > > + bool printable = true; > > + > > + for (i=0; i<size; i++) { > > + if (!isprint(buf[i])) { > > + printable = false; > > + break; > > + } > > + } > > + > > + if (isatty(STDOUT_FILENO) && !printable) { > > + hexdump(addr, buf, size, 1); > > + } else { > > + if (write(STDOUT_FILENO, buf, size) < 0) > > + PR_ERROR("Unable to write stdout.\n"); > > + } > > } > > > > free(buf); > > Amitay.
On Wednesday, 17 April 2019 3:26:00 PM AEST Amitay Isaacs wrote: > On Wed, 2019-04-17 at 15:20 +1000, Alistair Popple wrote: > > On Wednesday, 10 April 2019 6:01:50 PM AEST Amitay Isaacs wrote: > > > If getmem is being run on a terminal and if the output contains > > > non-printable characters, then use hexdump() to dump the data. > > > > To be honest really don't like this behavior being dependent on where > > stdout > > is going to. It seems like it would be suprising. For example if > > someone piped > > this into grep they would expect it to be running against the hexdump > > not the > > raw binary which could be confusing. > > Fair enough. So by default it will use hexdump() unless the buffer is > printable. And when given the extra flag, it'll be always binary. Sounds good! Thanks. - Alistair > > The printable character detection is a great idea though. Can we just > > add a > > flag to force raw binary output instead of doing it based on write > > location? > > > > - Alistair > > > > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > > > --- > > > > > > src/mem.c | 20 ++++++++++++++++++-- > > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/mem.c b/src/mem.c > > > index 8045069..b264d98 100644 > > > --- a/src/mem.c > > > +++ b/src/mem.c > > > @@ -21,6 +21,7 @@ > > > > > > #include <unistd.h> > > > #include <assert.h> > > > #include <stdbool.h> > > > > > > +#include <ctype.h> > > > > > > #include <libpdbg.h> > > > > > > @@ -28,6 +29,7 @@ > > > > > > #include "progress.h" > > > #include "optcmd.h" > > > #include "parsers.h" > > > > > > +#include "util.h" > > > > > > #define PR_ERROR(x, args...) \ > > > > > > pdbg_log(PDBG_ERROR, x, ##args) > > > > > > @@ -75,8 +77,22 @@ static int _getmem(uint64_t addr, uint64_t size, > > > uint8_t > > > block_size, bool ci) } > > > > > > if (count > 0) { > > > > > > - if (write(STDOUT_FILENO, buf, size) < 0) > > > - PR_ERROR("Unable to write stdout.\n"); > > > + uint64_t i; > > > + bool printable = true; > > > + > > > + for (i=0; i<size; i++) { > > > + if (!isprint(buf[i])) { > > > + printable = false; > > > + break; > > > + } > > > + } > > > + > > > + if (isatty(STDOUT_FILENO) && !printable) { > > > + hexdump(addr, buf, size, 1); > > > + } else { > > > + if (write(STDOUT_FILENO, buf, size) < 0) > > > + PR_ERROR("Unable to write stdout.\n"); > > > + } > > > > > > } > > > > > > free(buf); > > Amitay.
diff --git a/src/mem.c b/src/mem.c index 8045069..b264d98 100644 --- a/src/mem.c +++ b/src/mem.c @@ -21,6 +21,7 @@ #include <unistd.h> #include <assert.h> #include <stdbool.h> +#include <ctype.h> #include <libpdbg.h> @@ -28,6 +29,7 @@ #include "progress.h" #include "optcmd.h" #include "parsers.h" +#include "util.h" #define PR_ERROR(x, args...) \ pdbg_log(PDBG_ERROR, x, ##args) @@ -75,8 +77,22 @@ static int _getmem(uint64_t addr, uint64_t size, uint8_t block_size, bool ci) } if (count > 0) { - if (write(STDOUT_FILENO, buf, size) < 0) - PR_ERROR("Unable to write stdout.\n"); + uint64_t i; + bool printable = true; + + for (i=0; i<size; i++) { + if (!isprint(buf[i])) { + printable = false; + break; + } + } + + if (isatty(STDOUT_FILENO) && !printable) { + hexdump(addr, buf, size, 1); + } else { + if (write(STDOUT_FILENO, buf, size) < 0) + PR_ERROR("Unable to write stdout.\n"); + } } free(buf);
If getmem is being run on a terminal and if the output contains non-printable characters, then use hexdump() to dump the data. Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> --- src/mem.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)