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

[Daniel P. Berrange]

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 <address@hidden>
> > ---
> >  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 <address@hidden>



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 :|