| Message ID | 20170810160451.32723-9-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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; }
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(-)