[Qemu-devel] [PATCH v2] sockets: fix parsing of ipv4/ipv6 opts in parse_socket_addr

[Qemu-devel] [PATCH v2] sockets: fix parsing of ipv4/ipv6 opts in parse_socket_addr

[Daniel P. Berrangé]

From: "Daniel P. Berrange" <berrange@redhat.com>

The inet_parse() function looks for 'ipv4' and 'ipv6' flags, but only
treats them as bare bool flags. The normal QemuOpts parsing would allow
on/off values to be set too.

This updates inet_parse() so that its handling of the 'ipv4' and 'ipv6'
flags matches that done by QemuOpts.

This impacts the NBD block driver parsing the legacy filename syntax and
the migration code parsing the socket scheme.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 util/qemu-sockets.c | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index d6a1e1759e..300ebce795 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -554,6 +554,33 @@ err:
 }
 
 /* compatibility wrapper */
+static int inet_parse_flag(const char *flagname, const char *optstr, bool *val,
+                           Error **errp)
+{
+    char *end;
+    size_t len;
+
+    end = strstr(optstr, ",");
+    if (end) {
+        if (end[1] == ',') { /* Reject 'ipv6=on,,foo' */
+            error_setg(errp, "error parsing '%s' flag '%s'", flagname, optstr);
+            return -1;
+        }
+        len = end - optstr;
+    } else {
+        len = strlen(optstr);
+    }
+    if (len == 0 || (len == 3 && strncmp(optstr, "=on", len) == 0)) {
+        *val = true;
+    } else if ((len == 4) && strncmp(optstr, "=off", len) == 0) {
+        *val = false;
+    } else {
+        error_setg(errp, "error parsing '%s' flag '%s'", flagname, optstr);
+        return -1;
+    }
+    return 0;
+}
+
 int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
 {
     const char *optstr, *h;
@@ -561,6 +588,7 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
     char port[33];
     int to;
     int pos;
+    char *begin;
 
     memset(addr, 0, sizeof(*addr));
 
@@ -602,11 +630,19 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
         addr->has_to = true;
         addr->to = to;
     }
-    if (strstr(optstr, ",ipv4")) {
-        addr->ipv4 = addr->has_ipv4 = true;
+    begin = strstr(optstr, ",ipv4");
+    if (begin) {
+        if (inet_parse_flag("ipv4", begin + 5, &addr->ipv4, errp) < 0) {
+            return -1;
+        }
+        addr->has_ipv4 = true;
     }
-    if (strstr(optstr, ",ipv6")) {
-        addr->ipv6 = addr->has_ipv6 = true;
+    begin = strstr(optstr, ",ipv6");
+    if (begin) {
+        if (inet_parse_flag("ipv6", begin + 5, &addr->ipv6, errp) < 0) {
+            return -1;
+        }
+        addr->has_ipv6 = true;
     }
     return 0;
 }
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2] sockets: fix parsing of ipv4/ipv6 opts in parse_socket_addr

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

On 01/25/2018 11:14 AM, Daniel P. Berrangé wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> The inet_parse() function looks for 'ipv4' and 'ipv6' flags, but only
> treats them as bare bool flags. The normal QemuOpts parsing would allow
> on/off values to be set too.
> 
> This updates inet_parse() so that its handling of the 'ipv4' and 'ipv6'
> flags matches that done by QemuOpts.
> 
> This impacts the NBD block driver parsing the legacy filename syntax and
> the migration code parsing the socket scheme.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  util/qemu-sockets.c | 44 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 

> +    if (len == 0 || (len == 3 && strncmp(optstr, "=on", len) == 0)) {

Good: no parens around 'len == 3'

> +        *val = true;
> +    } else if ((len == 4) && strncmp(optstr, "=off", len) == 0) {

Not so good: redundant parens around 'len == 4'

Bad: Inconsistency between the two forms.

With those made consistent, this matches the logic in the static
util/qemu-option.c:parse_option_bool(), so it makes sense.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2] sockets: fix parsing of ipv4/ipv6 opts in parse_socket_addr

[Paolo Bonzini]

On 25/01/2018 18:14, Daniel P. Berrangé wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> The inet_parse() function looks for 'ipv4' and 'ipv6' flags, but only
> treats them as bare bool flags. The normal QemuOpts parsing would allow
> on/off values to be set too.
> 
> This updates inet_parse() so that its handling of the 'ipv4' and 'ipv6'
> flags matches that done by QemuOpts.
> 
> This impacts the NBD block driver parsing the legacy filename syntax and
> the migration code parsing the socket scheme.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  util/qemu-sockets.c | 44 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index d6a1e1759e..300ebce795 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -554,6 +554,33 @@ err:
>  }
>  
>  /* compatibility wrapper */
> +static int inet_parse_flag(const char *flagname, const char *optstr, bool *val,
> +                           Error **errp)
> +{
> +    char *end;
> +    size_t len;
> +
> +    end = strstr(optstr, ",");
> +    if (end) {
> +        if (end[1] == ',') { /* Reject 'ipv6=on,,foo' */
> +            error_setg(errp, "error parsing '%s' flag '%s'", flagname, optstr);
> +            return -1;
> +        }
> +        len = end - optstr;
> +    } else {
> +        len = strlen(optstr);
> +    }
> +    if (len == 0 || (len == 3 && strncmp(optstr, "=on", len) == 0)) {
> +        *val = true;
> +    } else if ((len == 4) && strncmp(optstr, "=off", len) == 0) {
> +        *val = false;
> +    } else {
> +        error_setg(errp, "error parsing '%s' flag '%s'", flagname, optstr);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
>  int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
>  {
>      const char *optstr, *h;
> @@ -561,6 +588,7 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
>      char port[33];
>      int to;
>      int pos;
> +    char *begin;
>  
>      memset(addr, 0, sizeof(*addr));
>  
> @@ -602,11 +630,19 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
>          addr->has_to = true;
>          addr->to = to;
>      }
> -    if (strstr(optstr, ",ipv4")) {
> -        addr->ipv4 = addr->has_ipv4 = true;
> +    begin = strstr(optstr, ",ipv4");
> +    if (begin) {
> +        if (inet_parse_flag("ipv4", begin + 5, &addr->ipv4, errp) < 0) {
> +            return -1;
> +        }
> +        addr->has_ipv4 = true;
>      }
> -    if (strstr(optstr, ",ipv6")) {
> -        addr->ipv6 = addr->has_ipv6 = true;
> +    begin = strstr(optstr, ",ipv6");
> +    if (begin) {
> +        if (inet_parse_flag("ipv6", begin + 5, &addr->ipv6, errp) < 0) {
> +            return -1;
> +        }
> +        addr->has_ipv6 = true;
>      }
>      return 0;
>  }
> 

Applied with Eric's suggested change.

Paolo