diff mbox series

net: remove qemu_ether_ntoa()

Message ID 20210226120607.675753-1-marcandre.lureau@redhat.com
State New
Headers show
Series net: remove qemu_ether_ntoa() | expand

Commit Message

Marc-André Lureau Feb. 26, 2021, 12:06 p.m. UTC
From: Marc-André Lureau <marcandre.lureau@redhat.com>

The function is not thread-safe and sets a bad example. It's used in a
single place for tracing, so open-code the format string like other
trace events with MAC addresses.

Cc: qemu-trivial@nongnu.org
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu-common.h |  1 -
 net/announce.c        |  8 +++++++-
 util/cutils.c         | 13 -------------
 net/trace-events      |  2 +-
 4 files changed, 8 insertions(+), 16 deletions(-)

Comments

Philippe Mathieu-Daudé Feb. 26, 2021, 12:52 p.m. UTC | #1
On 2/26/21 1:06 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The function is not thread-safe and sets a bad example. It's used in a
> single place for tracing, so open-code the format string like other
> trace events with MAC addresses.
> 
> Cc: qemu-trivial@nongnu.org
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu-common.h |  1 -
>  net/announce.c        |  8 +++++++-
>  util/cutils.c         | 13 -------------
>  net/trace-events      |  2 +-
>  4 files changed, 8 insertions(+), 16 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Doug Evans Feb. 26, 2021, 4:21 p.m. UTC | #2
On Fri, Feb 26, 2021 at 4:06 AM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The function is not thread-safe and sets a bad example. It's used in a
> single place for tracing, so open-code the format string like other
> trace events with MAC addresses.
>
> Cc: qemu-trivial@nongnu.org
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu-common.h |  1 -
>  net/announce.c        |  8 +++++++-
>  util/cutils.c         | 13 -------------
>  net/trace-events      |  2 +-
>  4 files changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 654621444e..209133bfca 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -132,7 +132,6 @@ void qemu_hexdump(FILE *fp, const char *prefix,
>   */
>  int parse_debug_env(const char *name, int max, int initial);
>
> -const char *qemu_ether_ntoa(const MACAddr *mac);
>  void page_size_init(void);
>
>  /* returns non-zero if dump is in progress, otherwise zero is
> diff --git a/net/announce.c b/net/announce.c
> index 26f057f5ee..fc0c6baace 100644
> --- a/net/announce.c
> +++ b/net/announce.c
> @@ -146,7 +146,13 @@ static void qemu_announce_self_iter(NICState *nic,
> void *opaque)
>
>      trace_qemu_announce_self_iter(timer->params.has_id ? timer->params.id
> : "_",
>                                    nic->ncs->name,
> -                                  qemu_ether_ntoa(&nic->conf->macaddr),
> skip);
> +                                  nic->conf->macaddr.a[0],
> +                                  nic->conf->macaddr.a[1],
> +                                  nic->conf->macaddr.a[2],
> +                                  nic->conf->macaddr.a[3],
> +                                  nic->conf->macaddr.a[4],
> +                                  nic->conf->macaddr.a[5],
> +                                  skip);
>
>      if (!skip) {
>          len = announce_self_create(buf, nic->conf->macaddr.a);
> diff --git a/util/cutils.c b/util/cutils.c
> index 70c7d6efbd..b5460a72b4 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -847,19 +847,6 @@ int parse_debug_env(const char *name, int max, int
> initial)
>      return debug;
>  }
>
> -/*
> - * Helper to print ethernet mac address
> - */
> -const char *qemu_ether_ntoa(const MACAddr *mac)
> -{
> -    static char ret[18];
> -
> -    snprintf(ret, sizeof(ret), "%02x:%02x:%02x:%02x:%02x:%02x",
> -             mac->a[0], mac->a[1], mac->a[2], mac->a[3], mac->a[4],
> mac->a[5]);
> -
> -    return ret;
> -}
> -
>  /*
>   * Return human readable string for size @val.
>   * @val can be anything that uint64_t allows (no more than "16 EiB").
> diff --git a/net/trace-events b/net/trace-events
> index bfaff7891d..07d6203602 100644
> --- a/net/trace-events
> +++ b/net/trace-events
> @@ -1,7 +1,7 @@
>  # See docs/devel/tracing.txt for syntax documentation.
>
>  # announce.c
> -qemu_announce_self_iter(const char *id, const char *name, const char
> *mac, int skip) "%s:%s:%s skip: %d"
> +qemu_announce_self_iter(const char *id, const char *name, char mac0, char
> mac1, char mac2, char mac3, char mac4, char mac5, int skip)
> "%s:%s:%02x:%02x:%02x:%02x:%02x:%02x skip: %d"
>  qemu_announce_timer_del(bool free_named, bool free_timer, char *id) "free
> named: %d free timer: %d id: %s"
>
>  # vhost-user.c
> --
> 2.29.0
>


It's pretty tedious to open code that.
How about instead change qemu_ether_ntoa to being thread-safe?

And, separately, add a bit of type safety and put eth addrs in a struct?
diff mbox series

Patch

diff --git a/include/qemu-common.h b/include/qemu-common.h
index 654621444e..209133bfca 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -132,7 +132,6 @@  void qemu_hexdump(FILE *fp, const char *prefix,
  */
 int parse_debug_env(const char *name, int max, int initial);
 
-const char *qemu_ether_ntoa(const MACAddr *mac);
 void page_size_init(void);
 
 /* returns non-zero if dump is in progress, otherwise zero is
diff --git a/net/announce.c b/net/announce.c
index 26f057f5ee..fc0c6baace 100644
--- a/net/announce.c
+++ b/net/announce.c
@@ -146,7 +146,13 @@  static void qemu_announce_self_iter(NICState *nic, void *opaque)
 
     trace_qemu_announce_self_iter(timer->params.has_id ? timer->params.id : "_",
                                   nic->ncs->name,
-                                  qemu_ether_ntoa(&nic->conf->macaddr), skip);
+                                  nic->conf->macaddr.a[0],
+                                  nic->conf->macaddr.a[1],
+                                  nic->conf->macaddr.a[2],
+                                  nic->conf->macaddr.a[3],
+                                  nic->conf->macaddr.a[4],
+                                  nic->conf->macaddr.a[5],
+                                  skip);
 
     if (!skip) {
         len = announce_self_create(buf, nic->conf->macaddr.a);
diff --git a/util/cutils.c b/util/cutils.c
index 70c7d6efbd..b5460a72b4 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -847,19 +847,6 @@  int parse_debug_env(const char *name, int max, int initial)
     return debug;
 }
 
-/*
- * Helper to print ethernet mac address
- */
-const char *qemu_ether_ntoa(const MACAddr *mac)
-{
-    static char ret[18];
-
-    snprintf(ret, sizeof(ret), "%02x:%02x:%02x:%02x:%02x:%02x",
-             mac->a[0], mac->a[1], mac->a[2], mac->a[3], mac->a[4], mac->a[5]);
-
-    return ret;
-}
-
 /*
  * Return human readable string for size @val.
  * @val can be anything that uint64_t allows (no more than "16 EiB").
diff --git a/net/trace-events b/net/trace-events
index bfaff7891d..07d6203602 100644
--- a/net/trace-events
+++ b/net/trace-events
@@ -1,7 +1,7 @@ 
 # See docs/devel/tracing.txt for syntax documentation.
 
 # announce.c
-qemu_announce_self_iter(const char *id, const char *name, const char *mac, int skip) "%s:%s:%s skip: %d"
+qemu_announce_self_iter(const char *id, const char *name, char mac0, char mac1, char mac2, char mac3, char mac4, char mac5, int skip) "%s:%s:%02x:%02x:%02x:%02x:%02x:%02x skip: %d"
 qemu_announce_timer_del(bool free_named, bool free_timer, char *id) "free named: %d free timer: %d id: %s"
 
 # vhost-user.c