From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55007) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEaFW-00055i-IB for qemu-devel@nongnu.org; Thu, 20 Sep 2012 02:28:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEaFV-0008ID-Bd for qemu-devel@nongnu.org; Thu, 20 Sep 2012 02:28:34 -0400 Received: from e28smtp01.in.ibm.com ([122.248.162.1]:50638) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEaFU-0008HE-Nj for qemu-devel@nongnu.org; Thu, 20 Sep 2012 02:28:33 -0400 Received: from /spool/local by e28smtp01.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 20 Sep 2012 11:58:29 +0530 Received: from d28av02.in.ibm.com (d28av02.in.ibm.com [9.184.220.64]) by d28relay04.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q8K6SP4k8585488 for ; Thu, 20 Sep 2012 11:58:26 +0530 Received: from d28av02.in.ibm.com (loopback [127.0.0.1]) by d28av02.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q8K6SPM3011938 for ; Thu, 20 Sep 2012 16:28:25 +1000 Date: Thu, 20 Sep 2012 12:00:19 +0530 From: Bharata B Rao Message-ID: <20120920063019.GC5873@in.ibm.com> References: <20120917152149.GB6879@in.ibm.com> <20120917152343.GD6879@in.ibm.com> <5058759C.50207@redhat.com> <505877CC.4040000@redhat.com> <5058806B.1050307@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5058806B.1050307@redhat.com> Subject: Re: [Qemu-devel] [PATCH v7 2/5] sockets: Change inet_parse() to accept address specification without port Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Anthony Liguori , Anand Avati , Vijay Bellur , Stefan Hajnoczi , Amar Tumballi , qemu-devel@nongnu.org, Markus Armbruster , Blue Swirl , Avi Kivity , Paolo Bonzini 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 > >>> > >>> 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.