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 <bhar...@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.
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