Re: [Qemu-devel] [PATCH v2 2/4] net: avoid using variable length array in net_client_init()

[Markus Armbruster <armbru@redhat.com>]

Stefano Garzarella <sgarzare@redhat.com> writes:

> net_client_init() uses a variable length array to store the prefix
> of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
> is 'fec0::0').
> This patch introduces g_strsplit() to split the 'ipv6-net' parameter,
> so we can remove the variable length array.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  net/net.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/net/net.c b/net/net.c
> index d5071e49e2..932fa5abb5 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1118,29 +1118,38 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
>          const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
>  
>          if (ip6_net) {
> -            char buf[strlen(ip6_net) + 1];
> +            gchar **substrings;
> +            char *prefix_addr;
> +            unsigned long prefix_len = 64; /* Default 64bit prefix length. */
>  
> -            if (get_str_sep(buf, sizeof(buf), &ip6_net, '/') < 0) {
> -                /* Default 64bit prefix length.  */
> -                qemu_opt_set(opts, "ipv6-prefix", ip6_net, &error_abort);
> -                qemu_opt_set_number(opts, "ipv6-prefixlen", 64, &error_abort);
> -            } else {
> +            substrings = g_strsplit(ip6_net, "/", 2);
> +            if (!substrings || !substrings[0]) {
> +                    error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                               "ipv6-net", "a valid IPv6 prefix");
> +                    g_strfreev(substrings);
> +                    goto out;

Indentation's off.

> +            }
> +
> +            *prefix_addr = substrings[0];
> +
> +            if (substrings[1]) {
>                  /* User-specified prefix length.  */
> -                unsigned long len;
>                  int err;
>  
> -                qemu_opt_set(opts, "ipv6-prefix", buf, &error_abort);
> -                err = qemu_strtoul(ip6_net, NULL, 10, &len);
> -
> +                err = qemu_strtoul(substrings[1], NULL, 10, &prefix_len);
>                  if (err) {
>                      error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                                 "ipv6-prefixlen", "a number");
> +                    g_strfreev(substrings);
>                      goto out;

Two g_strfreev() before goto out.  Avoidable: declare substrings at the
function level, initialize to NULL, then call g_strfreev(substrings) ...

>                  }
> -
> -                qemu_opt_set_number(opts, "ipv6-prefixlen", len, &error_abort);
>              }
> +
> +            qemu_opt_set(opts, "ipv6-prefix", prefix_addr, &error_abort);
> +            qemu_opt_set_number(opts, "ipv6-prefixlen", prefix_len,
> +                                &error_abort);
>              qemu_opt_unset(opts, "ipv6-net");
> +            g_strfreev(substrings);
>          }
>      }

       if (is_netdev) {
           visit_type_Netdev(v, NULL, (Netdev **)&object, &err);
       } else {
           visit_type_NetLegacy(v, NULL, (NetLegacy **)&object, &err);
       }

       if (!err) {
           ret = net_client_init1(object, is_netdev, &err);
       }

       if (is_netdev) {
           qapi_free_Netdev(object);
       } else {
           qapi_free_NetLegacy(object);
       }

   out:
       error_propagate(errp, err);

... here.  Your choice.

       visit_free(v);
       return ret;
   }

With at least the indentation fixed:
Reviewed-by: Markus Armbruster <armbru@redhat.com>


Not this patch's problem: when visit_type_FOO() fails with an input
visitor such as @v, you should not call qapi_free_FOO().  Nothing bad
happens when you do, it's just sloppy.  See visitor.h's big comment for
details.

Cleaner:

       if (is_netdev) {
           visit_type_Netdev(v, NULL, (Netdev **)&object, &err);
       } else {
           visit_type_NetLegacy(v, NULL, (NetLegacy **)&object, &err);
       }
       if (err) {
           goto out;
       }

       ret = net_client_init1(object, is_netdev, &err);

       if (is_netdev) {
           qapi_free_Netdev(object);
       } else {
           qapi_free_NetLegacy(object);
       }

   out:

Or maybe:

       if (is_netdev) {
           visit_type_Netdev(v, NULL, &netdev, &err);
           if (err) {
               goto out;
           }
           ret = net_client_init1(netdev, is_netdev, &err);
           qapi_free_Netdev(netdev);
       } else {
           visit_type_NetLegacy(v, NULL, &netlegacy, &err);
           if (err) {
               goto out;
           }
           ret = net_client_init1(netlegacy, is_netdev, &err);
           qapi_free_NetLegacy(netlegacy);
       }

   out:

with

       Netdev *netdev;
       NetLegacy *netlegacy;

replacing @object.

Or one step further: observe net_client_init() is always called with a
compile-time constant second argument.  Split it into two functions,
factor the common part into a helper.