Re: [PATCH v2 2/7] rocker: Revamp fp_port_get_info

From: Markus Armbruster <armbru@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>, Jiri Pirko <jiri@resnulli.us>,
	qemu-devel@nongnu.org, armbru@redhat.com
Subject: Re: [PATCH v2 2/7] rocker: Revamp fp_port_get_info
Date: Tue, 17 Nov 2020 10:27:09 +0100	[thread overview]
Message-ID: <87pn4c9vv6.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20201113011340.463563-3-eblake@redhat.com> (Eric Blake's message of "Thu, 12 Nov 2020 19:13:35 -0600")

Eric Blake <eblake@redhat.com> writes:

> Instead of modifying the value member of a list element passed as a
> parameter, and open-coding the manipulation of that list, it's nicer
> to just return a freshly allocated value to be prepended to a list
> using QAPI_LIST_PREPEND.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  hw/net/rocker/rocker_fp.h |  2 +-
>  hw/net/rocker/rocker.c    |  8 +-------
>  hw/net/rocker/rocker_fp.c | 17 ++++++++++-------
>  3 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/hw/net/rocker/rocker_fp.h b/hw/net/rocker/rocker_fp.h
> index dbe1dd329a4b..7ff57aac0180 100644
> --- a/hw/net/rocker/rocker_fp.h
> +++ b/hw/net/rocker/rocker_fp.h
> @@ -28,7 +28,7 @@ int fp_port_eg(FpPort *port, const struct iovec *iov, int iovcnt);
>
>  char *fp_port_get_name(FpPort *port);
>  bool fp_port_get_link_up(FpPort *port);
> -void fp_port_get_info(FpPort *port, RockerPortList *info);
> +RockerPort *fp_port_get_info(FpPort *port);
>  void fp_port_get_macaddr(FpPort *port, MACAddr *macaddr);
>  void fp_port_set_macaddr(FpPort *port, MACAddr *macaddr);
>  uint8_t fp_port_get_learning(FpPort *port);
> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> index 1af1e6fa2f9b..c53a02315e54 100644
> --- a/hw/net/rocker/rocker.c
> +++ b/hw/net/rocker/rocker.c
> @@ -127,13 +127,7 @@ RockerPortList *qmp_query_rocker_ports(const char *name, Error **errp)
>      }
>
>      for (i = r->fp_ports - 1; i >= 0; i--) {
> -        RockerPortList *info = g_malloc0(sizeof(*info));
> -        info->value = g_malloc0(sizeof(*info->value));
> -        struct fp_port *port = r->fp_port[i];
> -
> -        fp_port_get_info(port, info);
> -        info->next = list;
> -        list = info;
> +        QAPI_LIST_PREPEND(list, fp_port_get_info(r->fp_port[i]));

Relies on QAPI_LIST_PREPEND() evaluating its second argument exactly
once.  Part of its written contract; okay.

>      }
>
>      return list;
> diff --git a/hw/net/rocker/rocker_fp.c b/hw/net/rocker/rocker_fp.c
> index 4aa7da79b81d..cbeed65bd5ec 100644
> --- a/hw/net/rocker/rocker_fp.c
> +++ b/hw/net/rocker/rocker_fp.c
> @@ -51,14 +51,17 @@ bool fp_port_get_link_up(FpPort *port)
>      return !qemu_get_queue(port->nic)->link_down;
>  }
>
> -void fp_port_get_info(FpPort *port, RockerPortList *info)
> +RockerPort *fp_port_get_info(FpPort *port)
>  {
> -    info->value->name = g_strdup(port->name);
> -    info->value->enabled = port->enabled;
> -    info->value->link_up = fp_port_get_link_up(port);
> -    info->value->speed = port->speed;
> -    info->value->duplex = port->duplex;
> -    info->value->autoneg = port->autoneg;
> +    RockerPort *value = g_malloc0(sizeof(*value));
> +
> +    value->name = g_strdup(port->name);
> +    value->enabled = port->enabled;
> +    value->link_up = fp_port_get_link_up(port);
> +    value->speed = port->speed;
> +    value->duplex = port->duplex;
> +    value->autoneg = port->autoneg;
> +    return value;
>  }
>
>  void fp_port_get_macaddr(FpPort *port, MACAddr *macaddr)

Reviewed-by: Markus Armbruster <armbru@redhat.com>