On Mon, Jul 23, 2012 at 4:04 PM, Laszlo Ersek <ler...@redhat.com> 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 <ler...@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.
You're right, it can be done without searching the hubs/ports.
Thanks, I'll include that in the next version.
Stefan
