Re: [Qemu-block] [PATCH 8/8] sockets: fix parsing of ipv4/ipv6 opts in parse_socket_addr
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
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
> > ---
> > 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
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-block] [PATCH 8/8] sockets: fix parsing of ipv4/ipv6 opts in parse_socket_addr
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
> ---
> 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
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-block] [PATCH 8/8] sockets: fix parsing of ipv4/ipv6 opts in parse_socket_addr
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
---
tests/test-sockets-proto.c | 13 -
util/qemu-sockets.c| 36
2 files changed, 32 insertions(+), 17 deletions(-)
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,1
