From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:45252)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1TDxuQ-0005k7-5t
	for qemu-devel@nongnu.org; Tue, 18 Sep 2012 09:32:23 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1TDxuJ-00070d-Ih
	for qemu-devel@nongnu.org; Tue, 18 Sep 2012 09:32:14 -0400
Received: from mail-pb0-f45.google.com ([209.85.160.45]:40658)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1TDxuJ-00070H-BW
	for qemu-devel@nongnu.org; Tue, 18 Sep 2012 09:32:07 -0400
Received: by pbbrp12 with SMTP id rp12so11735711pbb.4
	for <qemu-devel@nongnu.org>; Tue, 18 Sep 2012 06:32:05 -0700 (PDT)
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <505877CC.4040000@redhat.com>
Date: Tue, 18 Sep 2012 15:31:56 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <20120917152149.GB6879@in.ibm.com>
	<20120917152343.GD6879@in.ibm.com> <5058759C.50207@redhat.com>
In-Reply-To: <5058759C.50207@redhat.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v7 2/5] sockets: Change inet_parse() to
 accept address specification without port
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Kevin Wolf <kwolf@redhat.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>, Anand Avati <aavati@redhat.com>, Vijay Bellur <vbellur@redhat.com>, Stefan Hajnoczi <stefanha@gmail.com>, Amar Tumballi <amarts@redhat.com>, qemu-devel@nongnu.org, Markus Armbruster <armbru@redhat.com>, Blue Swirl <blauwirbel@gmail.com>, Avi Kivity <avi@redhat.com>, bharata@linux.vnet.ibm.com

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

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.

Paolo

> Anyway, it does look correct.
> 
> Kevin
> 
>