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

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

[Stefano Garzarella]

This series contains some clean ups in net/net.c

The patch 1 solves an assertion failure when ipv6-prefixlen is not a number,

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.

v3:
 - Patch 2:
   - fix indentation [Markus]
   - move substrings at the function level, and call g_strfreev(substrings)
     at the end of the function [Markus]
 - add Markus' R-b

v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg615866.html
v1: 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 | 99 +++++++++++++++++++++++++++----------------------------
 1 file changed, 49 insertions(+), 50 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v3 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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 net/net.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/net.c b/net/net.c
index 2cf5e76469..555504a404 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1135,11 +1135,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");
         }
@@ -1161,6 +1161,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

[Qemu-devel] [PATCH v3 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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
v3:
  - fix indentation [Markus]
  - move substrings at the function level, and call g_strfreev(substrings)
    at the end of the function [Markus]
---
 net/net.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/net/net.c b/net/net.c
index 555504a404..47c03e5843 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1104,6 +1104,7 @@ static void show_netdevs(void)
 
 static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
 {
+    gchar **substrings = NULL;
     void *object = NULL;
     Error *err = NULL;
     int ret = -1;
@@ -1119,28 +1120,33 @@ 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];
+            char *prefix_addr;
+            unsigned long prefix_len = 64; /* Default 64bit prefix length. */
+
+            substrings = g_strsplit(ip6_net, "/", 2);
+            if (!substrings || !substrings[0]) {
+                error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "ipv6-net",
+                           "a valid IPv6 prefix");
+                goto out;
+            }
 
-            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 {
+            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");
                     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");
         }
     }
@@ -1163,6 +1169,7 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
 
 out:
     error_propagate(errp, err);
+    g_strfreev(substrings);
     visit_free(v);
     return ret;
 }
-- 
2.20.1

[Qemu-devel] [PATCH v3 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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 net/net.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/net/net.c b/net/net.c
index 47c03e5843..23f29ef1d2 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

[Qemu-devel] [PATCH v3 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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 net/net.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/net/net.c b/net/net.c
index 23f29ef1d2..4e1b25ba91 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 v3 0/4] Clean ups in net/net.c

[Stefano Garzarella]

Ping.

Thanks,
Stefano

On Fri, May 17, 2019 at 3:51 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> This series contains some clean ups in net/net.c
>
> The patch 1 solves an assertion failure when ipv6-prefixlen is not a number,
>
> 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.
>
> v3:
>  - Patch 2:
>    - fix indentation [Markus]
>    - move substrings at the function level, and call g_strfreev(substrings)
>      at the end of the function [Markus]
>  - add Markus' R-b
>
> v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg615866.html
> v1: 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 | 99 +++++++++++++++++++++++++++----------------------------
>  1 file changed, 49 insertions(+), 50 deletions(-)
>
> --
> 2.20.1
>
>

Re: [Qemu-devel] [PATCH v3 0/4] Clean ups in net/net.c

[Jason Wang]

On 2019/5/17 下午9:47, Stefano Garzarella wrote:
> This series contains some clean ups in net/net.c
>
> The patch 1 solves an assertion failure when ipv6-prefixlen is not a number,
>
> 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.
>
> v3:
>   - Patch 2:
>     - fix indentation [Markus]
>     - move substrings at the function level, and call g_strfreev(substrings)
>       at the end of the function [Markus]
>   - add Markus' R-b
>
> v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg615866.html
> v1: 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 | 99 +++++++++++++++++++++++++++----------------------------
>   1 file changed, 49 insertions(+), 50 deletions(-)
>

Applied.

Thanks