[8/8] sockets: fix parsing of ipv4/ipv6 opts in parse_socket_addr

Submitted by Daniel P. Berrange on Aug. 10, 2017, 4:04 p.m.

Details

Message ID 20170810160451.32723-9-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrange Aug. 10, 2017, 4:04 p.m.
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 updated inet_parse() so that its handling of the
'ipv4' and 'ipv6' flags matches that done by QemuOpts.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/test-sockets-proto.c | 13 -------------
 util/qemu-sockets.c        | 36 ++++++++++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 17 deletions(-)

Comments

Eric Blake Aug. 10, 2017, 6:35 p.m.
On 08/10/2017 11:04 AM, Daniel P. Berrange wrote:
> 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 updated inet_parse() so that its handling of the

s/updated/updates/ ?

> 'ipv4' and 'ipv6' flags matches that done by QemuOpts.

Do we have a regression compared to any previous version, such that this
patch might be considered 2.10 material?  Offhand, though, I think it's
fine as the end of your series, waiting for 2.11.

> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  tests/test-sockets-proto.c | 13 -------------
>  util/qemu-sockets.c        | 36 ++++++++++++++++++++++++++++++++----
>  2 files changed, 32 insertions(+), 17 deletions(-)
> 

> +++ b/util/qemu-sockets.c
> @@ -616,6 +616,25 @@ 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, ",");

Do we need to check that we are not landing on a ',,' escape that would
make QemuOpts behave differently?  [That is, ipv4=on,,garbage should be
parsed as setting ipv4 to 'on,garbage' (which is not valid), and NOT
setting 'ipv4=on' followed by the 'garbage' or ',garbage' key - while
the key named 'garbage' would fail, there might be other key names where
the distinction matters for catching command line typos]

But if this is unrelated to QemuOpts escape parsing, it seems okay.

Reviewed-by: Eric Blake <eblake@redhat.com>
Daniel P. Berrange Aug. 11, 2017, 9:22 a.m.
On Thu, Aug 10, 2017 at 01:35:15PM -0500, Eric Blake wrote:
> On 08/10/2017 11:04 AM, Daniel P. Berrange wrote:
> > 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 updated inet_parse() so that its handling of the
> 
> s/updated/updates/ ?
> 
> > 'ipv4' and 'ipv6' flags matches that done by QemuOpts.
> 
> Do we have a regression compared to any previous version, such that this
> patch might be considered 2.10 material?  Offhand, though, I think it's
> fine as the end of your series, waiting for 2.11.

Well this code has been like this for years, so waiting to fix
it in 2.11 isn't making our life any worse than it already
us.

The original code merely looks for a prefix so treats

   ,ipv6
   ,ipv6dumpsterfire
   ,ipv6=off
   ,ipv6=fishfood
   ,ipv6<anything>

as all meaning 'true'. The new code only treats ,ipv6 and ,ipv6=on
as meaning true, or ipv6=off as false, rejecting all others.

So yes, that is technically a regression, but IMHO it is a desirable
regression :-)

> 
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  tests/test-sockets-proto.c | 13 -------------
> >  util/qemu-sockets.c        | 36 ++++++++++++++++++++++++++++++++----
> >  2 files changed, 32 insertions(+), 17 deletions(-)
> > 
> 
> > +++ b/util/qemu-sockets.c
> > @@ -616,6 +616,25 @@ 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, ",");
> 
> Do we need to check that we are not landing on a ',,' escape that would
> make QemuOpts behave differently?  [That is, ipv4=on,,garbage should be
> parsed as setting ipv4 to 'on,garbage' (which is not valid), and NOT
> setting 'ipv4=on' followed by the 'garbage' or ',garbage' key - while
> the key named 'garbage' would fail, there might be other key names where
> the distinction matters for catching command line typos]
> 
> But if this is unrelated to QemuOpts escape parsing, it seems okay.

Ultimately this code should probably be converted to actually use
QemuOpts. The existing code already allows ipv4=on,,garbage but as
you point out I've not detected that case here. Should be easye
enough to fix though.

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



Regards,
Daniel

Patch hide | download patch | download mbox

diff --git a/tests/test-sockets-proto.c b/tests/test-sockets-proto.c
index a92389bef6..5805d2be5f 100644
--- a/tests/test-sockets-proto.c
+++ b/tests/test-sockets-proto.c
@@ -69,7 +69,6 @@  typedef struct {
  */
 static QSocketsData test_data[] = {
     /* Migrate with "" address */
-    /* XXX all settings with =off are disabled due to inet_parse() bug */
     { .ipv4 = 1, .ipv6 = 1, .error = false,
       .name = "/sockets/migrate/wildcard/all",
       .args = "-incoming tcp::9000" },
@@ -85,7 +84,6 @@  static QSocketsData test_data[] = {
     { .ipv4 = 0, .ipv6 = 1, .error = false,
       .name = "/sockets/migrate/wildcard/ipv6on",
       .args = "-incoming tcp::9000,ipv6=on" },
-    /*
     { .ipv4 = 0, .ipv6 = 1, .error = false,
       .name = "/sockets/migrate/wildcard/ipv4off",
       .args = "-incoming tcp::9000,ipv4=off" },
@@ -98,15 +96,12 @@  static QSocketsData test_data[] = {
     { .ipv4 = 0, .ipv6 = 1, .error = false,
       .name = "/sockets/migrate/wildcard/ipv4offipv6on",
       .args = "-incoming tcp::9000,ipv4=off,ipv6=on" },
-    */
     { .ipv4 = 1, .ipv6 = 1, .error = false,
       .name = "/sockets/migrate/wildcard/ipv4onipv6on",
       .args = "-incoming tcp::9000,ipv4=on,ipv6=on" },
-    /*
     { .ipv4 = 0, .ipv6 = 0, .error = true,
       .name = "/sockets/migrate/wildcard/ipv4offipv6off",
       .args = "-incoming tcp::9000,ipv4=off,ipv6=off" },
-    */
 
     /* Migrate with 0.0.0.0 address */
     { .ipv4 = 1, .ipv6 = 0, .error = false,
@@ -124,7 +119,6 @@  static QSocketsData test_data[] = {
     { .ipv4 = 0, .ipv6 = 0, .error = true,
       .name = "/sockets/migrate/0.0.0.0/ipv6on",
       .args = "-incoming tcp:0.0.0.0:9000,ipv6=on" },
-    /*
     { .ipv4 = 0, .ipv6 = 0, .error = true,
       .name = "/sockets/migrate/0.0.0.0/ipv4off",
       .args = "-incoming tcp:0.0.0.0:9000,ipv4=off" },
@@ -137,15 +131,12 @@  static QSocketsData test_data[] = {
     { .ipv4 = 0, .ipv6 = 0, .error = true,
       .name = "/sockets/migrate/0.0.0.0/ipv4offipv6on",
       .args = "-incoming tcp:0.0.0.0:9000,ipv4=off,ipv6=on" },
-    */
     { .ipv4 = 1, .ipv6 = 0, .error = false,
       .name = "/sockets/migrate/0.0.0.0/ipv4onipv6on",
       .args = "-incoming tcp:0.0.0.0:9000,ipv4=on,ipv6=on" },
-    /*
     { .ipv4 = 0, .ipv6 = 0, .error = true,
       .name = "/sockets/migrate/0.0.0.0/ipv4offipv6off",
       .args = "-incoming tcp:0.0.0.0:9000,ipv4=off,ipv6=off" },
-    */
 
     /* Migrate with :: address */
     { .ipv4 = 1, .ipv6 = 1, .error = false,
@@ -163,7 +154,6 @@  static QSocketsData test_data[] = {
     { .ipv4 = 0, .ipv6 = 1, .error = false,
       .name = "/sockets/migrate/::/ipv6on",
       .args = "-incoming tcp:[::]:9000,ipv6=on" },
-    /*
     { .ipv4 = 0, .ipv6 = 1, .error = false,
       .name = "/sockets/migrate/::/ipv4off",
       .args = "-incoming tcp:[::]:9000,ipv4=off" },
@@ -176,15 +166,12 @@  static QSocketsData test_data[] = {
     { .ipv4 = 0, .ipv6 = 1, .error = false,
       .name = "/sockets/migrate/::/ipv4offipv6on",
       .args = "-incoming tcp:[::]:9000,ipv4=off,ipv6=on" },
-    */
     { .ipv4 = 1, .ipv6 = 1, .error = false,
       .name = "/sockets/migrate/::/ipv4onipv6on",
       .args = "-incoming tcp:[::]:9000,ipv4=on,ipv6=on" },
-    /*
     { .ipv4 = 0, .ipv6 = 0, .error = true,
       .name = "/sockets/migrate/::/ipv4offipv6off",
       .args = "-incoming tcp:[::]:9000,ipv4=off,ipv6=off" },
-    */
 
 
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 1358c81bcc..76202949f5 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -616,6 +616,25 @@  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, ",");
+    len = end ? end - optstr : 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;
@@ -623,6 +642,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));
 
@@ -664,11 +684,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;
 }