From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:59842)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1StKB2-0006Ej-Vf
	for qemu-devel@nongnu.org; Mon, 23 Jul 2012 11:04:06 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1StKAt-0001fy-EL
	for qemu-devel@nongnu.org; Mon, 23 Jul 2012 11:04:04 -0400
Received: from mx1.redhat.com ([209.132.183.28]:27768)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1StKAt-0001ft-5X
	for qemu-devel@nongnu.org; Mon, 23 Jul 2012 11:03:55 -0400
Message-ID: <500D67F0.5000809@redhat.com>
Date: Mon, 23 Jul 2012 17:04:16 +0200
From: Laszlo Ersek <lersek@redhat.com>
MIME-Version: 1.0
References: <1342785709-3152-1-git-send-email-stefanha@linux.vnet.ibm.com>
	<1342785709-3152-4-git-send-email-stefanha@linux.vnet.ibm.com>
In-Reply-To: <1342785709-3152-4-git-send-email-stefanha@linux.vnet.ibm.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 03/16] net: Look up 'vlan' net clients
	using hubs
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: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>, qemu-devel@nongnu.org, Zhi Yong Wu <wuzhy@cn.ibm.com>

On 07/20/12 14:01, Stefan Hajnoczi wrote:

> @@ -679,8 +680,10 @@ void do_info_usernet(Monitor *mon)
>      SlirpState *s;
>  
>      QTAILQ_FOREACH(s, &slirp_stacks, entry) {
> +        unsigned int id;
> +        bool got_vlan_id = net_hub_id_for_client(&s->nc, &id) == 0;
>          monitor_printf(mon, "VLAN %d (%s):\n",
> -                       s->nc.vlan ? s->nc.vlan->id : -1,
> +                       got_vlan_id ? id : -1,
>                         s->nc.name);
>          slirp_connection_info(s->slirp, mon);
>      }


This patch seems OK, so let me quickly posit:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

But this hunk made me think again about net_hub_id_for_client(). The
above lookup used to take  a constant number of pointer dereferences,
now it takes a loop in a loop.

Before:

                         VLANClientState
                               |
          VLANClientState -- VLAN -- VLANClientState
                               |
                         VLANClientState

Now:

                          "HubPortPeer"
                               |
                            HubPort
                               |
  "HubPortPeer" -- HubPort -- Hub -- HubPort -- "HubPortPeer"
                               |
                            HubPort
                               |
                          "HubPortPeer"

net_hub_id_for_client() seems to provide two searches in one: it can key
off the hub port itself on the hub, but it can also key off the port's
peer (the true net device).

Couldn't we just implement net_hub_id_for_client() as follows (still
providing both searches)? qemu_new_net_client() sets the peer links both
ways.

    int
    net_hub_id_for_client(VLANClientState *nc, unsigned int *id)
    {
        NetHubPort *port;

        if (nc->info->type == NET_CLIENT_OPTIONS_KIND_HUBPORT) {
            port = DO_UPCAST(NetHubPort, nc, nc);
        }
        else if (nc->peer != NULL && nc->peer->info->type ==
                 NET_CLIENT_OPTIONS_KIND_HUBPORT) {
            port = DO_UPCAST(NetHubPort, nc, nc->peer);
        }
        else {
            return -ENOENT;
        }

        *id = port->hub->id;
        return 0;
    }

Sorry if this is a dumb question, but I have to understand if I want to
review the series sensibily.

Thanks!
Laszlo