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

[Kevin Wolf]

Am 20.09.2012 08:30, schrieb Bharata B Rao:
> 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 <address@hidden>
>>>>>
>>>>> 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.

Ah, so you're talking about a potential problem after making the port
optional, not about a real, existing bug in the code as it is today?

Please rephrase the commit message then; actually I wouldn't even care
to talk about the reason for the scanf changes, they are obviously
required for making the port optional.

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

Yes, but make it 'int default_port'.

You could make default_port = -1 retain the current behaviour, i.e. port
is required in str (document this in a comment before inet_listen). This
would probably be the right thing to pass for existing callers.

Kevin