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

[Stefano Garzarella]

On Thu, May 16, 2019 at 10:44:20AM +0200, Markus Armbruster wrote:
> Stefano Garzarella <address@hidden> 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 <address@hidden>
> > Signed-off-by: Stefano Garzarella <address@hidden>
> > ---
> >  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.
> 

Copy and past issue :(

> > +            }
> > +
> > +            *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) ...o

I'll fix it in the v3, it's cleaner.

> 
> >                  }
> > -
> > -                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 <address@hidden>

Thanks!

> 
> 
> 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.

Yeah, I think could be better!
I'll create an helper to parse the IPv6 prefix and two separated
functions to initialize Netdev or NetLegacy.

I'll put the new patch in the v3, are you agree?

Thanks,
Stefano