Re: [Qemu-devel] [PATCH 03/16] net: Look up 'vlan' net clients using hubs

[Laszlo Ersek <lersek@redhat.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