diff mbox series

[v2,2/4] net: avoid using variable length array in net_client_init()

Message ID 20190509130345.227526-3-sgarzare@redhat.com (mailing list archive)
State New, archived
Headers show
Series Clean ups in net/net.c | expand

Commit Message

Stefano Garzarella May 9, 2019, 1:03 p.m. UTC
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(-)

Comments

Markus Armbruster May 16, 2019, 8:44 a.m. UTC | #1
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.
Stefano Garzarella May 16, 2019, 11:41 a.m. UTC | #2
On Thu, May 16, 2019 at 10:44:20AM +0200, Markus Armbruster wrote:
> 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.
> 

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 <armbru@redhat.com>

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
diff mbox series

Patch

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