[PATCH] util/hexdump: Convert to take a void pointer argument

Philippe Mathieu-Daudé posted 1 patch 3 years, 8 months ago
Test docker-quick@centos7 failed
Test docker-mingw@fedora failed
Test checkpatch failed
Test FreeBSD failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200822150457.1322519-1-f4bug@amsat.org
There is a newer version of this series
include/qemu-common.h    |  2 +-
hw/dma/xlnx_dpdma.c      |  2 +-
hw/net/fsl_etsec/etsec.c |  2 +-
hw/sd/sd.c               |  2 +-
hw/usb/redirect.c        |  2 +-
net/colo-compare.c       | 12 ++++++------
net/net.c                |  2 +-
util/hexdump.c           |  3 ++-
8 files changed, 14 insertions(+), 13 deletions(-)
[PATCH] util/hexdump: Convert to take a void pointer argument
Posted by Philippe Mathieu-Daudé 3 years, 8 months ago
Most uses of qemu_hexdump() do not take an array of char
as input, forcing use of cast. Since we can use this
helper to dump any kind of buffer, use a pointer to void
argument instead.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qemu-common.h    |  2 +-
 hw/dma/xlnx_dpdma.c      |  2 +-
 hw/net/fsl_etsec/etsec.c |  2 +-
 hw/sd/sd.c               |  2 +-
 hw/usb/redirect.c        |  2 +-
 net/colo-compare.c       | 12 ++++++------
 net/net.c                |  2 +-
 util/hexdump.c           |  3 ++-
 8 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index bb9496bd80f..de82a619abd 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -138,7 +138,7 @@ int os_parse_cmd_args(int index, const char *optarg);
  * Hexdump a buffer to a file. An optional string prefix is added to every line
  */
 
-void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size);
+void qemu_hexdump(const void *ptr, FILE *fp, const char *prefix, size_t size);
 
 /*
  * helper to parse debug environment variables
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index b40c897de2c..d75a8069426 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -388,7 +388,7 @@ static void xlnx_dpdma_dump_descriptor(DPDMADescriptor *desc)
 {
     if (DEBUG_DPDMA) {
         qemu_log("DUMP DESCRIPTOR:\n");
-        qemu_hexdump((char *)desc, stdout, "", sizeof(DPDMADescriptor));
+        qemu_hexdump(desc, stdout, "", sizeof(DPDMADescriptor));
     }
 }
 
diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index 7035cf4eb97..c817a28decd 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -357,7 +357,7 @@ static ssize_t etsec_receive(NetClientState *nc,
 
 #if defined(HEX_DUMP)
     fprintf(stderr, "%s receive size:%zd\n", nc->name, size);
-    qemu_hexdump((void *)buf, stderr, "", size);
+    qemu_hexdump(buf, stderr, "", size);
 #endif
     /* Flush is unnecessary as are already in receiving path */
     etsec->need_flush = false;
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index fad9cf1ee7a..190e4cf1232 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1781,7 +1781,7 @@ send_response:
     }
 
 #ifdef DEBUG_SD
-    qemu_hexdump((const char *)response, stderr, "Response", rsplen);
+    qemu_hexdump(response, stderr, "Response", rsplen);
 #endif
 
     return rsplen;
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 417a60a2e68..09edb0d81c0 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -240,7 +240,7 @@ static void usbredir_log_data(USBRedirDevice *dev, const char *desc,
     if (dev->debug < usbredirparser_debug_data) {
         return;
     }
-    qemu_hexdump((char *)data, stderr, desc, len);
+    qemu_hexdump(data, stderr, desc, len);
 }
 
 /*
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 2c20de1537d..550272b3baa 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -494,9 +494,9 @@ sec:
         g_queue_push_head(&conn->secondary_list, spkt);
 
         if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
-            qemu_hexdump((char *)ppkt->data, stderr,
+            qemu_hexdump(ppkt->data, stderr,
                         "colo-compare ppkt", ppkt->size);
-            qemu_hexdump((char *)spkt->data, stderr,
+            qemu_hexdump(spkt->data, stderr,
                         "colo-compare spkt", spkt->size);
         }
 
@@ -535,9 +535,9 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
         trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
         trace_colo_compare_udp_miscompare("Secondary pkt size", spkt->size);
         if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
-            qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
+            qemu_hexdump(ppkt->data, stderr, "colo-compare pri pkt",
                          ppkt->size);
-            qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
+            qemu_hexdump(spkt->data, stderr, "colo-compare sec pkt",
                          spkt->size);
         }
         return -1;
@@ -578,9 +578,9 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
         trace_colo_compare_icmp_miscompare("Secondary pkt size",
                                            spkt->size);
         if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
-            qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
+            qemu_hexdump(ppkt->data, stderr, "colo-compare pri pkt",
                          ppkt->size);
-            qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
+            qemu_hexdump(spkt->data, stderr, "colo-compare sec pkt",
                          spkt->size);
         }
         return -1;
diff --git a/net/net.c b/net/net.c
index bbaedb3c7a6..f3e5d533fd7 100644
--- a/net/net.c
+++ b/net/net.c
@@ -636,7 +636,7 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
 
 #ifdef DEBUG_NET
     printf("qemu_send_packet_async:\n");
-    qemu_hexdump((const char *)buf, stdout, "net", size);
+    qemu_hexdump(buf, stdout, "net", size);
 #endif
 
     if (sender->link_down || !sender->peer) {
diff --git a/util/hexdump.c b/util/hexdump.c
index f879ff0ad6b..faff78970a4 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -16,8 +16,9 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 
-void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size)
+void qemu_hexdump(const void *ptr, FILE *fp, const char *prefix, size_t size)
 {
+    const char *buf = ptr;
     unsigned int b, len, i, c;
 
     for (b = 0; b < size; b += 16) {
-- 
2.26.2


Re: [PATCH] util/hexdump: Convert to take a void pointer argument
Posted by Peter Maydell 3 years, 8 months ago
On Sat, 22 Aug 2020 at 16:05, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Most uses of qemu_hexdump() do not take an array of char
> as input, forcing use of cast. Since we can use this
> helper to dump any kind of buffer, use a pointer to void
> argument instead.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> -void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size);
> +void qemu_hexdump(const void *ptr, FILE *fp, const char *prefix, size_t size);

Changing the type seems reasonable, but it is still a pointer
to a buffer, so I think keeping the name 'buf' is more descriptive.

As an aside, is it just me that finds the order of arguments
here a bit odd? The pointer to the buffer and the length of
the buffer are closely related arguments that are widely
separated in the argument list order, and the FILE* that
you might expect to come first doesn't. "fp, prefix, buf, size"
would seem more logical. Not sure it's worth the effort of
changing, though...

thanks
-- PMM

Re: [PATCH] util/hexdump: Convert to take a void pointer argument
Posted by Philippe Mathieu-Daudé 3 years, 8 months ago

On 8/22/20 6:18 PM, Peter Maydell wrote:
> On Sat, 22 Aug 2020 at 16:05, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Most uses of qemu_hexdump() do not take an array of char
>> as input, forcing use of cast. Since we can use this
>> helper to dump any kind of buffer, use a pointer to void
>> argument instead.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
>> -void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size);
>> +void qemu_hexdump(const void *ptr, FILE *fp, const char *prefix, size_t size);
> 
> Changing the type seems reasonable, but it is still a pointer
> to a buffer, so I think keeping the name 'buf' is more descriptive.

I'll use 'bufptr' so I don't have to modify the function body.

> 
> As an aside, is it just me that finds the order of arguments
> here a bit odd? The pointer to the buffer and the length of
> the buffer are closely related arguments that are widely
> separated in the argument list order, and the FILE* that
> you might expect to come first doesn't. "fp, prefix, buf, size"
> would seem more logical. Not sure it's worth the effort of
> changing, though...

Can do that. I expect this function to eventually disappear,
as there are many efforts to not directly write to stdout/stderr
(this function support generic FILE, but all callers use stdio).

> 
> thanks
> -- PMM
>