diff mbox

[03/16] net: Look up 'vlan' net clients using hubs

Message ID 1342785709-3152-4-git-send-email-stefanha@linux.vnet.ibm.com
State New
Headers show

Commit Message

Stefan Hajnoczi July 20, 2012, 12:01 p.m. UTC 
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 net.c       |   28 +---------------------------
 net/hub.c   |   24 ++++++++++++++++++++++++
 net/hub.h   |    2 ++
 net/slirp.c |    7 +++++--
 4 files changed, 32 insertions(+), 29 deletions(-)

Comments

Laszlo Ersek July 23, 2012, 3:04 p.m. UTC | #1 
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
Stefan Hajnoczi July 23, 2012, 3:21 p.m. UTC | #2 
On Mon, Jul 23, 2012 at 4:04 PM, Laszlo Ersek <lersek@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 <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.

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

Stefan
diff mbox

Patch

diff --git a/net.c b/net.c
index f2752c7..2a8c5c5 100644
--- a/net.c
+++ b/net.c
@@ -316,32 +316,6 @@  void qemu_del_vlan_client(VLANClientState *vc)
     qemu_free_vlan_client(vc);
 }
 
-VLANClientState *
-qemu_find_vlan_client_by_name(Monitor *mon, int vlan_id,
-                              const char *client_str)
-{
-    VLANState *vlan;
-    VLANClientState *vc;
-
-    vlan = qemu_find_vlan(vlan_id, 0);
-    if (!vlan) {
-        monitor_printf(mon, "unknown VLAN %d\n", vlan_id);
-        return NULL;
-    }
-
-    QTAILQ_FOREACH(vc, &vlan->clients, next) {
-        if (!strcmp(vc->name, client_str)) {
-            break;
-        }
-    }
-    if (!vc) {
-        monitor_printf(mon, "can't find device %s on VLAN %d\n",
-                       client_str, vlan_id);
-    }
-
-    return vc;
-}
-
 void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
 {
     VLANClientState *nc;
@@ -995,7 +969,7 @@  void net_host_device_remove(Monitor *mon, const QDict *qdict)
     int vlan_id = qdict_get_int(qdict, "vlan_id");
     const char *device = qdict_get_str(qdict, "device");
 
-    vc = qemu_find_vlan_client_by_name(mon, vlan_id, device);
+    vc = net_hub_find_client_by_name(vlan_id, device);
     if (!vc) {
         return;
     }
diff --git a/net/hub.c b/net/hub.c
index e0ce476..104dd4f 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -165,6 +165,30 @@  VLANClientState *net_hub_add_port(unsigned int hub_id, const char *name)
 }
 
 /**
+ * Find a specific client on a hub
+ */
+VLANClientState *net_hub_find_client_by_name(unsigned int hub_id,
+                                             const char *name)
+{
+    NetHub *hub;
+    NetHubPort *port;
+    VLANClientState *peer;
+
+    QLIST_FOREACH(hub, &hubs, next) {
+        if (hub->id == hub_id) {
+            QLIST_FOREACH(port, &hub->ports, next) {
+                peer = port->nc.peer;
+
+                if (peer && strcmp(peer->name, name) == 0) {
+                    return peer;
+                }
+            }
+        }
+    }
+    return NULL;
+}
+
+/**
  * Print hub configuration
  */
 void net_hub_info(Monitor *mon)
diff --git a/net/hub.h b/net/hub.h
index b642c13..1b9db52 100644
--- a/net/hub.h
+++ b/net/hub.h
@@ -20,6 +20,8 @@ 
 int net_init_hubport(const NetClientOptions *opts, const char *name,
                      VLANClientState *peer);
 VLANClientState *net_hub_add_port(unsigned int hub_id, const char *name);
+VLANClientState *net_hub_find_client_by_name(unsigned int hub_id,
+                                             const char *name);
 void net_hub_info(Monitor *mon);
 int net_hub_id_for_client(VLANClientState *nc, unsigned int *id);
 
diff --git a/net/slirp.c b/net/slirp.c
index 97e380c..c753b02 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -30,6 +30,7 @@ 
 #include <sys/wait.h>
 #endif
 #include "net.h"
+#include "net/hub.h"
 #include "monitor.h"
 #include "qemu_socket.h"
 #include "slirp/libslirp.h"
@@ -284,7 +285,7 @@  static SlirpState *slirp_lookup(Monitor *mon, const char *vlan,
 
     if (vlan) {
         VLANClientState *nc;
-        nc = qemu_find_vlan_client_by_name(mon, strtol(vlan, NULL, 0), stack);
+        nc = net_hub_find_client_by_name(strtol(vlan, NULL, 0), stack);
         if (!nc) {
             return NULL;
         }
@@ -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);
     }