| Message ID | fcfc06335c0f3eaecacd2ac8396c232142ff4774.1406707320.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
31.07.2014 04:31, Peter Crosthwaite wrote: > To avoid callsites with optional output having to NULL guard. Isn't it a bit backwards? If we don't need output, maybe we should not call hexdump in the first place? Thanks, /mjt > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > --- > Noting in-tree is affected by this yet, but I though I'd get this > out of the way straight-up rather than elongate other series. > > util/hexdump.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/util/hexdump.c b/util/hexdump.c > index 969b340..b607236 100644 > --- a/util/hexdump.c > +++ b/util/hexdump.c > @@ -19,6 +19,9 @@ void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size) > { > unsigned int b; > > + if (!fp) { > + return; > + } > for (b = 0; b < size; b++) { > if ((b % 16) == 0) { > fprintf(fp, "%s: %04x:", prefix, b); >
On Sat, Aug 2, 2014 at 11:26 PM, Michael Tokarev <mjt@tls.msk.ru> wrote: > 31.07.2014 04:31, Peter Crosthwaite wrote: >> To avoid callsites with optional output having to NULL guard. > > Isn't it a bit backwards? If we don't need output, maybe we should > not call hexdump in the first place? > Well my thinking is a NULL File * is a good way to "don't need output" and then it means then the multiple callsites can avoid: if (fp) { Note that many of the exitinst > Thanks, > > /mjt > >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> --- >> Noting in-tree is affected by this yet, but I though I'd get this >> out of the way straight-up rather than elongate other series. >> >> util/hexdump.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/util/hexdump.c b/util/hexdump.c >> index 969b340..b607236 100644 >> --- a/util/hexdump.c >> +++ b/util/hexdump.c >> @@ -19,6 +19,9 @@ void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size) >> { >> unsigned int b; >> >> + if (!fp) { >> + return; >> + } >> for (b = 0; b < size; b++) { >> if ((b % 16) == 0) { >> fprintf(fp, "%s: %04x:", prefix, b); >> > >
Sorry premature send (I guess I just discovered a new keyboard shortcut!) On Sun, Aug 3, 2014 at 11:14 AM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Sat, Aug 2, 2014 at 11:26 PM, Michael Tokarev <mjt@tls.msk.ru> wrote: >> 31.07.2014 04:31, Peter Crosthwaite wrote: >>> To avoid callsites with optional output having to NULL guard. >> >> Isn't it a bit backwards? If we don't need output, maybe we should >> not call hexdump in the first place? >> > > Well my thinking is a NULL File * is a good way to "don't need output" > and then it means then the multiple callsites can avoid: > > > if (fp) { > > > Note that many of the exitinst > > > >> Thanks, >> >> /mjt >> >>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>> --- >>> Noting in-tree is affected by this yet, but I though I'd get this >>> out of the way straight-up rather than elongate other series. >>> >>> util/hexdump.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/util/hexdump.c b/util/hexdump.c >>> index 969b340..b607236 100644 >>> --- a/util/hexdump.c >>> +++ b/util/hexdump.c >>> @@ -19,6 +19,9 @@ void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size) >>> { >>> unsigned int b; >>> >>> + if (!fp) { >>> + return; >>> + } >>> for (b = 0; b < size; b++) { >>> if ((b % 16) == 0) { >>> fprintf(fp, "%s: %04x:", prefix, b); >>> >> >>
On Sun, Aug 3, 2014 at 11:14 AM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Sat, Aug 2, 2014 at 11:26 PM, Michael Tokarev <mjt@tls.msk.ru> wrote: >> 31.07.2014 04:31, Peter Crosthwaite wrote: >>> To avoid callsites with optional output having to NULL guard. >> >> Isn't it a bit backwards? If we don't need output, maybe we should >> not call hexdump in the first place? >> > Well my thinking is a NULL File * is a good way to "don't need output" and then it means then the multiple callsites can avoid: if (fp) { qemu_hexdump(foo, fp, ..) } Note that many of the existing call this with stderr as FP argument which in theory should be change to something that backs onto qemu_log. I suspect the file handle in this case will be NULL when logging is disabled and this patch avoid having to add all these conditionals. Regards, Peter >> Thanks, >> >> /mjt >> >>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>> --- >>> Noting in-tree is affected by this yet, but I though I'd get this >>> out of the way straight-up rather than elongate other series. >>> >>> util/hexdump.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/util/hexdump.c b/util/hexdump.c >>> index 969b340..b607236 100644 >>> --- a/util/hexdump.c >>> +++ b/util/hexdump.c >>> @@ -19,6 +19,9 @@ void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size) >>> { >>> unsigned int b; >>> >>> + if (!fp) { >>> + return; >>> + } >>> for (b = 0; b < size; b++) { >>> if ((b % 16) == 0) { >>> fprintf(fp, "%s: %04x:", prefix, b); >>> >> >>
On 3 August 2014 02:17, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > Note that many of the existing call this with stderr as FP argument > which in theory should be change to something that backs onto > qemu_log. I suspect the file handle in this case will be NULL when > logging is disabled and this patch avoid having to add all these > conditionals. For the qemu_log case we should handle the "don't log" condition in the qemu_log() wrapper, especially since we probably want the wrapper to take a log-mask anyway. thanks -- PMM
diff --git a/util/hexdump.c b/util/hexdump.c index 969b340..b607236 100644 --- a/util/hexdump.c +++ b/util/hexdump.c @@ -19,6 +19,9 @@ void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size) { unsigned int b; + if (!fp) { + return; + } for (b = 0; b < size; b++) { if ((b % 16) == 0) { fprintf(fp, "%s: %04x:", prefix, b);
To avoid callsites with optional output having to NULL guard. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- Noting in-tree is affected by this yet, but I though I'd get this out of the way straight-up rather than elongate other series. util/hexdump.c | 3 +++ 1 file changed, 3 insertions(+)