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

[Stefan Hajnoczi]

On Mon, Jul 23, 2012 at 4:04 PM, Laszlo Ersek <address@hidden> wrote:
> 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 <address@hidden>
>
> 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.

You're right, it can be done without searching the hubs/ports.
Thanks, I'll include that in the next version.

Stefan