[Qemu-devel] [PATCH v2 0/4] Clean ups in net/net.c

[Qemu-devel] [PATCH v2 0/4] Clean ups in net/net.c

[Stefano Garzarella]

This series is a follow up of "[PATCH] net: avoid to use variable length
array in net_client_init()" [1], so it contains the v2 of that patch,
plus other new related patches.

I discovered an assertion failure while I was testing the patches, so I
added the patch 1 to solve this issue.

Following the Markus' advice, I modified the parsing of IPv6 prefix
(patch 2) and IPv4 host:port (patch 3). Then I removed the get_str_sep()
function (patch 4) because it is no longer used.

@Markus, I modified a little bit what you suggested, introducing
g_strsplit() in order to de-duplicate the qemu_opt_set() and
qemu_opt_set_number(). I hope it's good for you.

v2:
 - Added patches 1,3 and 4
 - Patch 2:
   - clean up parsing of IPv6 prefix [Markus]
   - fixed subject line [Eric]

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg614561.html

Stefano Garzarella (4):
  net: fix assertion failure when ipv6-prefixlen is not a number
  net: avoid using variable length array in net_client_init()
  net: use g_strsplit() for parsing host address and port
  net: remove unused get_str_sep() function

 net/net.c | 101 +++++++++++++++++++++++++++---------------------------
 1 file changed, 51 insertions(+), 50 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/4] net: fix assertion failure when ipv6-prefixlen is not a number

[Stefano Garzarella]

If 'ipv6-prefixlen' is not a number, the current behaviour
produces an assertion failure:
    $ qemu-system-x86_64 -net user,ipv6-net=feca::0/a
    qemu-system-x86_64: qemu/util/qemu-option.c:1175: qemu_opts_foreach:
    Assertion `!errp || !*errp' failed.
    Aborted (core dumped)

This patch fixes it, jumping to the end of the function when
'ipv6-prefixlen' is not a number, and printing the more friendly
message:
    $ qemu-system-x86_64 -net user,ipv6-net=feca::0/a
    qemu-system-x86_64: Parameter 'ipv6-prefixlen' expects a number

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 net/net.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/net.c b/net/net.c
index f3a3c5444c..d5071e49e2 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1134,11 +1134,11 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
 
                 if (err) {
                     error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                              "ipv6-prefix", "a number");
-                } else {
-                    qemu_opt_set_number(opts, "ipv6-prefixlen", len,
-                                        &error_abort);
+                               "ipv6-prefixlen", "a number");
+                    goto out;
                 }
+
+                qemu_opt_set_number(opts, "ipv6-prefixlen", len, &error_abort);
             }
             qemu_opt_unset(opts, "ipv6-net");
         }
@@ -1160,6 +1160,7 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
         qapi_free_NetLegacy(object);
     }
 
+out:
     error_propagate(errp, err);
     visit_free(v);
     return ret;
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 1/4] net: fix assertion failure when ipv6-prefixlen is not a number

[Markus Armbruster]

Stefano Garzarella <sgarzare@redhat.com> writes:

> If 'ipv6-prefixlen' is not a number, the current behaviour
> produces an assertion failure:
>     $ qemu-system-x86_64 -net user,ipv6-net=feca::0/a
>     qemu-system-x86_64: qemu/util/qemu-option.c:1175: qemu_opts_foreach:
>     Assertion `!errp || !*errp' failed.
>     Aborted (core dumped)
>
> This patch fixes it, jumping to the end of the function when
> 'ipv6-prefixlen' is not a number, and printing the more friendly
> message:
>     $ qemu-system-x86_64 -net user,ipv6-net=feca::0/a
>     qemu-system-x86_64: Parameter 'ipv6-prefixlen' expects a number
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

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

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

[Stefano Garzarella]

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

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

[Markus Armbruster]

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.

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

[Qemu-devel] [PATCH v2 3/4] net: use g_strsplit() for parsing host address and port

[Stefano Garzarella]

Use the glib function to split host address and port in
the parse_host_port() function.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 net/net.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/net/net.c b/net/net.c
index 932fa5abb5..570e093c4f 100644
--- a/net/net.c
+++ b/net/net.c
@@ -86,32 +86,39 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
 int parse_host_port(struct sockaddr_in *saddr, const char *str,
                     Error **errp)
 {
-    char buf[512];
+    gchar **substrings;
     struct hostent *he;
-    const char *p, *r;
-    int port;
+    const char *addr, *p, *r;
+    int port, ret = 0;
 
-    p = str;
-    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+    substrings = g_strsplit(str, ":", 2);
+    if (!substrings || !substrings[0] || !substrings[1]) {
         error_setg(errp, "host address '%s' doesn't contain ':' "
                    "separating host from port", str);
-        return -1;
+        ret = -1;
+        goto out;
     }
+
+    addr = substrings[0];
+    p = substrings[1];
+
     saddr->sin_family = AF_INET;
-    if (buf[0] == '\0') {
+    if (addr[0] == '\0') {
         saddr->sin_addr.s_addr = 0;
     } else {
-        if (qemu_isdigit(buf[0])) {
-            if (!inet_aton(buf, &saddr->sin_addr)) {
+        if (qemu_isdigit(addr[0])) {
+            if (!inet_aton(addr, &saddr->sin_addr)) {
                 error_setg(errp, "host address '%s' is not a valid "
-                           "IPv4 address", buf);
-                return -1;
+                           "IPv4 address", addr);
+                ret = -1;
+                goto out;
             }
         } else {
-            he = gethostbyname(buf);
+            he = gethostbyname(addr);
             if (he == NULL) {
-                error_setg(errp, "can't resolve host address '%s'", buf);
-                return - 1;
+                error_setg(errp, "can't resolve host address '%s'", addr);
+                ret = -1;
+                goto out;
             }
             saddr->sin_addr = *(struct in_addr *)he->h_addr;
         }
@@ -119,10 +126,14 @@ int parse_host_port(struct sockaddr_in *saddr, const char *str,
     port = strtol(p, (char **)&r, 0);
     if (r == p) {
         error_setg(errp, "port number '%s' is invalid", p);
-        return -1;
+        ret = -1;
+        goto out;
     }
     saddr->sin_port = htons(port);
-    return 0;
+
+out:
+    g_strfreev(substrings);
+    return ret;
 }
 
 char *qemu_mac_strdup_printf(const uint8_t *macaddr)
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 3/4] net: use g_strsplit() for parsing host address and port

[Markus Armbruster]

Stefano Garzarella <sgarzare@redhat.com> writes:

> Use the glib function to split host address and port in
> the parse_host_port() function.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

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

[Qemu-devel] [PATCH v2 4/4] net: remove unused get_str_sep() function

[Stefano Garzarella]

Since the get_str_sep() function is no longer used in
net/net.c, we can remove it.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 net/net.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/net/net.c b/net/net.c
index 570e093c4f..52496caca5 100644
--- a/net/net.c
+++ b/net/net.c
@@ -63,26 +63,6 @@ static QTAILQ_HEAD(, NetClientState) net_clients;
 /***********************************************************/
 /* network device redirectors */
 
-static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
-{
-    const char *p, *p1;
-    int len;
-    p = *pp;
-    p1 = strchr(p, sep);
-    if (!p1)
-        return -1;
-    len = p1 - p;
-    p1++;
-    if (buf_size > 0) {
-        if (len > buf_size - 1)
-            len = buf_size - 1;
-        memcpy(buf, p, len);
-        buf[len] = '\0';
-    }
-    *pp = p1;
-    return 0;
-}
-
 int parse_host_port(struct sockaddr_in *saddr, const char *str,
                     Error **errp)
 {
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 4/4] net: remove unused get_str_sep() function

[Markus Armbruster]

Stefano Garzarella <sgarzare@redhat.com> writes:

> Since the get_str_sep() function is no longer used in
> net/net.c, we can remove it.
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

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