Re: [Qemu-devel] [PATCH v7 2/5] sockets: Change inet_parse() to accept address specification without port

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Tue, Sep 18, 2012 at 04:08:43PM +0200, Kevin Wolf wrote:
> Am 18.09.2012 15:31, schrieb Paolo Bonzini:
> > Il 18/09/2012 15:22, Kevin Wolf ha scritto:
> >> Am 17.09.2012 17:23, schrieb Bharata B Rao:
> >>> sockets: Change inet_parse() to accept address specification without port
> >>>
> >>> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >>>
> >>> inet_parse() expects address:port. Change it to work without explicit port
> >>> specification. In addition, don't depend solely on the return value of
> >>
> >> Things like "in addition" in a commit message are almost always a sign
> >> that the patch should be split in two.

Both kind of go together. Not depending on return value of sscanf gives us
the ability to have the port as optional parameter. Will rephrase the patch
description accordingly.

> >>
> >>> sscanf but also consider the value obtained for %n directive used in sscanf.
> >>> This ensures that the scanning of malformed inet address isn't flagged as
> >>> success.
> >>
> >> Can you give an example string that would be falsely accepted? To me the
> >> old checks look fine (even though the new ones are a little bit easier
> >> to read, so even if they don't fix anything, they might be worth doing).
> > 
> > "localhost" would fail to be parsed:
> > 
> > -        if (2 != sscanf(str,"%64[^:]:%32[^,]%n",addr,port,&pos)) {
> > +        ret = sscanf(str, "%64[^:]%n:%32[^,]%n", addr, &addr_pos,
> > +            port, &port_pos);
> > +        if (addr_pos == -1 || ret == EOF) {
> > 
> > because the : in the format string would not match and sscanf would
> > return 1.
> 
> Yes, that's the part with making the port optional.
> 
> Bharata also claims that "scanning of malformed inet address" could
> falsely succeed before, which I can't see (but which I suspect is what
> the first two hunks of the patch are meant to address).

For malformed ipv6 address like "[1:2:3:4:5", sccanf in inet_parse
returns 1 (which means 1 input item successfully matched and assigned)
The current inet_parse code would eventually fail it since it checks for
return value of 2, but when I am making port optional, I can't depend
on return value of 1 or 2 since sscanf can return 1 for such incomplete ipv6
addresses too. Note that in the above case, though sscanf returned 1, the
pos argument remains unchanged indicating that it couldn't really parse
any input correctly.

So in summary, when I said scanning of malformed inet address succeeded
earlier, I should have been more specific by saying that sscanf in inet_parse
could return success for malformed ipv6 strings.

> 
> > However, is it correct to set the port unconditionally to an empty
> > string?  Your usecase makes sense, but perhaps the default port be
> > passed as an extra parameter to inet_parse instead.
> 
> I thought about this, too, but didn't care enough to mention it. Now
> that we're two, yes, I'd like adding a default port parameter.

So you are saying that lets change inet_parse to look like this:

int inet_parse(QemuOpts *opts, const char *str, int port)

and if @str didn't specify a port explicitly, use @port to populate
the port option in @opts ? Other callers of inet_parse are inet_listen and
inet_connect. What should be the default port values from these callers ?

Regards,
Bharata.