From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:47415)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1StJ1I-00016W-FJ
	for qemu-devel@nongnu.org; Mon, 23 Jul 2012 09:50:02 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1StJ1C-00047i-Kc
	for qemu-devel@nongnu.org; Mon, 23 Jul 2012 09:49:56 -0400
Received: from mail-lb0-f173.google.com ([209.85.217.173]:65102)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1StJ1C-00047X-Dy
	for qemu-devel@nongnu.org; Mon, 23 Jul 2012 09:49:50 -0400
Received: by lbok6 with SMTP id k6so8641108lbo.4
	for <qemu-devel@nongnu.org>; Mon, 23 Jul 2012 06:49:49 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <500D4754.2020301@redhat.com>
References: <1342785709-3152-1-git-send-email-stefanha@linux.vnet.ibm.com>
	<1342785709-3152-2-git-send-email-stefanha@linux.vnet.ibm.com>
	<500D4754.2020301@redhat.com>
Date: Mon, 23 Jul 2012 14:49:49 +0100
Message-ID: <CAJSP0QUpnp5TSjk9fXj1x02aJAUx_+6GBC7nDddGrV+A0wK27w@mail.gmail.com>
From: Stefan Hajnoczi <stefanha@gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Subject: Re: [Qemu-devel] [PATCH 01/16] net: Add a hub net client
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: Laszlo Ersek <lersek@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>, Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>, Zhi Yong Wu <wuzhy@cn.ibm.com>, qemu-devel@nongnu.org

On Mon, Jul 23, 2012 at 1:45 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> Two hairs to split:
>
> On 07/20/12 14:01, Stefan Hajnoczi wrote:
>
>> +static NetHubPort *net_hub_port_new(NetHub *hub, const char *name)
>> +{
>> +    VLANClientState *nc;
>> +    NetHubPort *port;
>> +    unsigned int id = hub->num_ports++;
>
> There are projects that don't like to put logic or externally visible
> side-effects into initializers. I don't know about qemu.

I see what you're saying, we also add it to the hub's port list
further down.  It could be split into _new() and hub_add_port(hub,
port) but then autogenerating a unique name becomes harder.  Since
this function is static, the scope is limited and we can assume
callers understand what is going on here.

I'd like to leave it this way or do you see a concrete change that
improves the code?

>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index bc55ed2..6618eb5 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2094,6 +2094,19 @@
>>      '*helper': 'str' } }
>>
>>  ##
>> +# @NetdevHubPortOptions
>> +#
>> +# Connect two or more net clients through a software hub.
>> +#
>> +# @hubid: hub identifier number
>> +#
>> +# Since 1.2
>> +##
>> +{ 'type': 'NetdevHubPortOptions',
>> +  'data': {
>> +    'hubid':     'int' } }
>
> I think this should say 'uint32'.

Okay.

Stefan