[Qemu-devel] [PATCH 0/7] Various small networking improvements

[Qemu-devel] [PATCH 0/7] Various small networking improvements

[Jan Kiszka]

This series fixes several smaller issues in the networking code and then
enhances it by two features:
 - VLAN capturing (derived from Tristan Gringold's work)
 - Handling of requested IP in slirp's DHCP server

The first four patches should be considered for stable, so I prepared a
corresponding queue at git://git.kiszka.org/qemu.git queues/stable.

Find this series also at git://git.kiszka.org/qemu.git queues/net

Jan Kiszka (7):
      net: Fix -net socket,listen
      net: Add VLAN client cleanup handler
      net: Check device passed to host_net_remove
      net: Prevent multiple slirp instances
      monitor: Improve host_net_add
      net: Add support for capturing VLANs
      slirp: Handle DHCP requests for specific IP

 hw/e1000.c          |    2 +-
 hw/eepro100.c       |    2 +-
 hw/etraxfs_eth.c    |    3 +-
 hw/mcf_fec.c        |    3 +-
 hw/mipsnet.c        |    3 +-
 hw/musicpal.c       |    2 +-
 hw/ne2000.c         |    4 +-
 hw/pcnet.c          |    3 +-
 hw/rtl8139.c        |    3 +-
 hw/smc91c111.c      |    3 +-
 hw/stellaris_enet.c |    3 +-
 hw/usb-net.c        |    2 +-
 hw/virtio-net.c     |    3 +-
 monitor.c           |    4 +-
 net.c               |  330 +++++++++++++++++++++++++++++++++++++++-----------
 net.h               |   11 ++
 qemu-options.hx     |    7 +
 slirp/bootp.c       |  127 ++++++++++++++------
 slirp/bootp.h       |    2 +
 19 files changed, 394 insertions(+), 123 deletions(-)

[Qemu-devel] [PATCH 3/7] net: Check device passed to host_net_remove

[Jan Kiszka]

Make sure that we do not delete guest NICs via host_net_remove.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 net.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/net.c b/net.c
index 36c0509..787f249 100644
--- a/net.c
+++ b/net.c
@@ -1861,9 +1861,16 @@ void net_host_device_remove(Monitor *mon, int vlan_id, const char *device)
         return;
     }
 
-   for(vc = vlan->first_client; vc != NULL; vc = vc->next)
-        if (!strcmp(vc->name, device))
+    if (!net_host_check_device(device)) {
+        monitor_printf(mon, "invalid host network device %s\n", device);
+        return;
+    }
+
+    for (vc = vlan->first_client; vc != NULL; vc = vc->next) {
+        if (!strcmp(vc->name, device)) {
             break;
+        }
+    }
 
     if (!vc) {
         monitor_printf(mon, "can't find device %s\n", device);

Re: [Qemu-devel] [PATCH 3/7] net: Check device passed to host_net_remove

[Mark McLoughlin]

On Tue, 2009-04-14 at 19:29 +0200, Jan Kiszka wrote:
> Make sure that we do not delete guest NICs via host_net_remove.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  net.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/net.c b/net.c
> index 36c0509..787f249 100644
> --- a/net.c
> +++ b/net.c
> @@ -1861,9 +1861,16 @@ void net_host_device_remove(Monitor *mon, int vlan_id, const char *device)
>          return;
>      }
>  
> -   for(vc = vlan->first_client; vc != NULL; vc = vc->next)
> -        if (!strcmp(vc->name, device))
> +    if (!net_host_check_device(device)) {
> +        monitor_printf(mon, "invalid host network device %s\n", device);
> +        return;
> +    }

Doesn't this mean that if you assign a name with e.g. "name=foo1234" you
won't be able to remove it?

Probably makes more sense to find the client, then check vc->model using
net_host_check_device()?

Cheers,
Mark.

[Qemu-devel] Re: [PATCH 3/7] net: Check device passed to host_net_remove

[Jan Kiszka]

Mark McLoughlin wrote:
> On Tue, 2009-04-14 at 19:29 +0200, Jan Kiszka wrote:
>> Make sure that we do not delete guest NICs via host_net_remove.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>>  net.c |   11 +++++++++--
>>  1 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/net.c b/net.c
>> index 36c0509..787f249 100644
>> --- a/net.c
>> +++ b/net.c
>> @@ -1861,9 +1861,16 @@ void net_host_device_remove(Monitor *mon, int vlan_id, const char *device)
>>          return;
>>      }
>>  
>> -   for(vc = vlan->first_client; vc != NULL; vc = vc->next)
>> -        if (!strcmp(vc->name, device))
>> +    if (!net_host_check_device(device)) {
>> +        monitor_printf(mon, "invalid host network device %s\n", device);
>> +        return;
>> +    }
> 
> Doesn't this mean that if you assign a name with e.g. "name=foo1234" you
> won't be able to remove it?

Good point.

> 
> Probably makes more sense to find the client, then check vc->model using
> net_host_check_device()?

Yes, will fix.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 2/7] net: Add VLAN client cleanup handler

[Jan Kiszka]

Do proper VLAN client cleanup via a callback handler. This fixes
resource leakage on host_net_remove and allows a generic net_cleanup
implementation.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 hw/e1000.c          |    2 -
 hw/eepro100.c       |    2 -
 hw/etraxfs_eth.c    |    3 +
 hw/mcf_fec.c        |    3 +
 hw/mipsnet.c        |    3 +
 hw/musicpal.c       |    2 -
 hw/ne2000.c         |    4 +
 hw/pcnet.c          |    3 +
 hw/rtl8139.c        |    3 +
 hw/smc91c111.c      |    3 +
 hw/stellaris_enet.c |    3 +
 hw/usb-net.c        |    2 -
 hw/virtio-net.c     |    3 +
 net.c               |  140 +++++++++++++++++++++++++++++----------------------
 net.h               |    4 +
 15 files changed, 106 insertions(+), 74 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 1644201..9ce116c 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1096,7 +1096,7 @@ pci_e1000_init(PCIBus *bus, NICInfo *nd, int devfn)
     memset(&d->tx, 0, sizeof d->tx);
 
     d->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                 e1000_receive, e1000_can_receive, d);
+                                 e1000_receive, e1000_can_receive, NULL, d);
     d->vc->link_status_changed = e1000_set_link_status;
 
     qemu_format_nic_info_str(d->vc, d->nd->macaddr);
diff --git a/hw/eepro100.c b/hw/eepro100.c
index 0a343df..321bd48 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1751,7 +1751,7 @@ static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd,
     nic_reset(s);
 
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                 nic_receive, nic_can_receive, s);
+                                 nic_receive, nic_can_receive, NULL, s);
 
     qemu_format_nic_info_str(s->vc, s->macaddr);
 
diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
index c87e55f..0d5fa9b 100644
--- a/hw/etraxfs_eth.c
+++ b/hw/etraxfs_eth.c
@@ -585,7 +585,8 @@ void *etraxfs_eth_init(NICInfo *nd, CPUState *env,
 	cpu_register_physical_memory (base, 0x5c, eth->ethregs);
 
 	eth->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-				       eth_receive, eth_can_receive, eth);
+				       eth_receive, eth_can_receive,
+				       NULL, eth);
 	eth->vc->opaque = eth;
 	eth->vc->link_status_changed = eth_set_link;
 
diff --git a/hw/mcf_fec.c b/hw/mcf_fec.c
index 413c569..6a42faf 100644
--- a/hw/mcf_fec.c
+++ b/hw/mcf_fec.c
@@ -455,7 +455,8 @@ void mcf_fec_init(NICInfo *nd, target_phys_addr_t base, qemu_irq *irq)
     cpu_register_physical_memory(base, 0x400, iomemtype);
 
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                 mcf_fec_receive, mcf_fec_can_receive, s);
+                                 mcf_fec_receive, mcf_fec_can_receive,
+                                 NULL, s);
     memcpy(s->macaddr, nd->macaddr, 6);
     qemu_format_nic_info_str(s->vc, s->macaddr);
 }
diff --git a/hw/mipsnet.c b/hw/mipsnet.c
index 29bd9b8..81739ac 100644
--- a/hw/mipsnet.c
+++ b/hw/mipsnet.c
@@ -251,7 +251,8 @@ void mipsnet_init (int base, qemu_irq irq, NICInfo *nd)
     s->nd = nd;
     if (nd && nd->vlan) {
         s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                     mipsnet_receive, mipsnet_can_receive, s);
+                                     mipsnet_receive, mipsnet_can_receive,
+                                     NULL, s);
     } else {
         s->vc = NULL;
     }
diff --git a/hw/musicpal.c b/hw/musicpal.c
index 5de1691..bdff03e 100644
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -755,7 +755,7 @@ static void mv88w8618_eth_init(NICInfo *nd, uint32_t base, qemu_irq irq)
     s = qemu_mallocz(sizeof(mv88w8618_eth_state));
     s->irq = irq;
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                 eth_receive, eth_can_receive, s);
+                                 eth_receive, eth_can_receive, NULL, s);
     iomemtype = cpu_register_io_memory(0, mv88w8618_eth_readfn,
                                        mv88w8618_eth_writefn, s);
     cpu_register_physical_memory(base, MP_ETH_SIZE, iomemtype);
diff --git a/hw/ne2000.c b/hw/ne2000.c
index 24a66bb..02cf5e3 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -742,7 +742,7 @@ void isa_ne2000_init(int base, qemu_irq irq, NICInfo *nd)
     ne2000_reset(s);
 
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                 ne2000_receive, ne2000_can_receive, s);
+                                 ne2000_receive, ne2000_can_receive, NULL, s);
 
     qemu_format_nic_info_str(s->vc, s->macaddr);
 
@@ -802,7 +802,7 @@ PCIDevice *pci_ne2000_init(PCIBus *bus, NICInfo *nd, int devfn)
     memcpy(s->macaddr, nd->macaddr, 6);
     ne2000_reset(s);
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                 ne2000_receive, ne2000_can_receive, s);
+                                 ne2000_receive, ne2000_can_receive, NULL, s);
 
     qemu_format_nic_info_str(s->vc, s->macaddr);
 
diff --git a/hw/pcnet.c b/hw/pcnet.c
index be68f28..ff1d1c3 100644
--- a/hw/pcnet.c
+++ b/hw/pcnet.c
@@ -1937,7 +1937,8 @@ static void pcnet_common_init(PCNetState *d, NICInfo *nd)
 
     if (nd && nd->vlan) {
         d->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                     pcnet_receive, pcnet_can_receive, d);
+                                     pcnet_receive, pcnet_can_receive,
+                                     NULL, d);
 
         qemu_format_nic_info_str(d->vc, d->nd->macaddr);
     } else {
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 9fa69db..a2b28bb 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -3450,7 +3450,8 @@ PCIDevice *pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
     memcpy(s->macaddr, nd->macaddr, 6);
     rtl8139_reset(s);
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                 rtl8139_receive, rtl8139_can_receive, s);
+                                 rtl8139_receive, rtl8139_can_receive,
+                                 NULL, s);
 
     qemu_format_nic_info_str(s->vc, s->macaddr);
 
diff --git a/hw/smc91c111.c b/hw/smc91c111.c
index f5b29a7..f20c9b3 100644
--- a/hw/smc91c111.c
+++ b/hw/smc91c111.c
@@ -707,7 +707,8 @@ void smc91c111_init(NICInfo *nd, uint32_t base, qemu_irq irq)
     smc91c111_reset(s);
 
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                 smc91c111_receive, smc91c111_can_receive, s);
+                                 smc91c111_receive, smc91c111_can_receive,
+                                 NULL, s);
     qemu_format_nic_info_str(s->vc, s->macaddr);
     /* ??? Save/restore.  */
 }
diff --git a/hw/stellaris_enet.c b/hw/stellaris_enet.c
index 88c5620..3035734 100644
--- a/hw/stellaris_enet.c
+++ b/hw/stellaris_enet.c
@@ -400,7 +400,8 @@ void stellaris_enet_init(NICInfo *nd, uint32_t base, qemu_irq irq)
 
     if (nd->vlan) {
         s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                     stellaris_enet_receive, stellaris_enet_can_receive, s);
+                                     stellaris_enet_receive,
+                                     stellaris_enet_can_receive, NULL, s);
         qemu_format_nic_info_str(s->vc, s->macaddr);
     }
 
diff --git a/hw/usb-net.c b/hw/usb-net.c
index 863c25f..2fd5112 100644
--- a/hw/usb-net.c
+++ b/hw/usb-net.c
@@ -1452,7 +1452,7 @@ USBDevice *usb_net_init(NICInfo *nd)
     pstrcpy(s->dev.devname, sizeof(s->dev.devname),
                     "QEMU USB Network Interface");
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                    usbnet_receive, usbnet_can_receive, s);
+                                 usbnet_receive, usbnet_can_receive, NULL, s);
 
     qemu_format_nic_info_str(s->vc, s->mac);
 
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 88ec1ac..bf15d39 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -603,7 +603,8 @@ PCIDevice *virtio_net_init(PCIBus *bus, NICInfo *nd, int devfn)
     memcpy(n->mac, nd->macaddr, ETH_ALEN);
     n->status = VIRTIO_NET_S_LINK_UP;
     n->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                 virtio_net_receive, virtio_net_can_receive, n);
+                                 virtio_net_receive, virtio_net_can_receive,
+                                 NULL, n);
     n->vc->link_status_changed = virtio_net_set_link_status;
 
     qemu_format_nic_info_str(n->vc, n->mac);
diff --git a/net.c b/net.c
index c7e4a0b..36c0509 100644
--- a/net.c
+++ b/net.c
@@ -333,6 +333,7 @@ VLANClientState *qemu_new_vlan_client(VLANState *vlan,
                                       const char *name,
                                       IOReadHandler *fd_read,
                                       IOCanRWHandler *fd_can_read,
+                                      VLANClientCleanupHandler *cleanup,
                                       void *opaque)
 {
     VLANClientState *vc, **pvc;
@@ -344,6 +345,7 @@ VLANClientState *qemu_new_vlan_client(VLANState *vlan,
         vc->name = assign_name(vc, model);
     vc->fd_read = fd_read;
     vc->fd_can_read = fd_can_read;
+    vc->cleanup = cleanup;
     vc->opaque = opaque;
     vc->vlan = vlan;
 
@@ -362,6 +364,9 @@ void qemu_del_vlan_client(VLANClientState *vc)
     while (*pvc != NULL)
         if (*pvc == vc) {
             *pvc = vc->next;
+            if (vc->cleanup) {
+                vc->cleanup(vc->opaque);
+            }
             free(vc->name);
             free(vc->model);
             free(vc);
@@ -521,7 +526,7 @@ static int net_slirp_init(VLANState *vlan, const char *model, const char *name)
         slirp_init(slirp_restrict, slirp_ip);
     }
     slirp_vc = qemu_new_vlan_client(vlan, model, name,
-                                    slirp_receive, NULL, NULL);
+                                    slirp_receive, NULL, NULL, NULL);
     slirp_vc->info_str[0] = '\0';
     return 0;
 }
@@ -752,6 +757,51 @@ static void tap_send(void *opaque)
 
 /* fd support */
 
+static int launch_script(const char *setup_script, const char *ifname, int fd)
+{
+    int pid, status, open_max, i;
+    char *args[3];
+    char **parg;
+
+    /* try to launch network script */
+    pid = fork();
+    if (pid < 0)
+        return -1;
+    if (pid == 0) {
+        open_max = sysconf(_SC_OPEN_MAX);
+        for (i = 0; i < open_max; i++) {
+            if (i != STDIN_FILENO && i != STDOUT_FILENO &&
+                i != STDERR_FILENO && i != fd) {
+                close(i);
+            }
+        }
+        parg = args;
+        *parg++ = (char *)setup_script;
+        *parg++ = (char *)ifname;
+        *parg++ = NULL;
+        execv(setup_script, args);
+        _exit(1);
+    }
+    while (waitpid(pid, &status, 0) != pid);
+    if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
+        fprintf(stderr, "%s: could not launch network script\n",
+                setup_script);
+        return -1;
+    }
+    return 0;
+}
+
+static void net_tap_cleanup(void *opaque)
+{
+    TAPState *s = opaque;
+
+    if (s->down_script[0]) {
+        launch_script(s->down_script, s->down_script_arg, s->fd);
+    }
+    close(s->fd);
+    qemu_free(s);
+}
+
 static TAPState *net_tap_fd_init(VLANState *vlan,
                                  const char *model,
                                  const char *name,
@@ -761,7 +811,8 @@ static TAPState *net_tap_fd_init(VLANState *vlan,
 
     s = qemu_mallocz(sizeof(TAPState));
     s->fd = fd;
-    s->vc = qemu_new_vlan_client(vlan, model, name, tap_receive, NULL, s);
+    s->vc = qemu_new_vlan_client(vlan, model, name, tap_receive, NULL,
+                                 net_tap_cleanup, s);
 #ifdef HAVE_IOVEC
     s->vc->fd_readv = tap_receive_iov;
 #endif
@@ -960,42 +1011,6 @@ static int tap_open(char *ifname, int ifname_size)
 }
 #endif
 
-static int launch_script(const char *setup_script, const char *ifname, int fd)
-{
-    int pid, status;
-    char *args[3];
-    char **parg;
-
-        /* try to launch network script */
-        pid = fork();
-        if (pid >= 0) {
-            if (pid == 0) {
-                int open_max = sysconf (_SC_OPEN_MAX), i;
-                for (i = 0; i < open_max; i++)
-                    if (i != STDIN_FILENO &&
-                        i != STDOUT_FILENO &&
-                        i != STDERR_FILENO &&
-                        i != fd)
-                        close(i);
-
-                parg = args;
-                *parg++ = (char *)setup_script;
-                *parg++ = (char *)ifname;
-                *parg++ = NULL;
-                execv(setup_script, args);
-                _exit(1);
-            }
-            while (waitpid(pid, &status, 0) != pid);
-            if (!WIFEXITED(status) ||
-                WEXITSTATUS(status) != 0) {
-                fprintf(stderr, "%s: could not launch network script\n",
-                        setup_script);
-                return -1;
-            }
-        }
-    return 0;
-}
-
 static int net_tap_init(VLANState *vlan, const char *model,
                         const char *name, const char *ifname1,
                         const char *setup_script, const char *down_script)
@@ -1064,6 +1079,14 @@ static void vde_from_qemu(void *opaque, const uint8_t *buf, int size)
     }
 }
 
+static void net_vde_cleanup(void *opaque)
+{
+    VDEState *s = opaque;
+
+    vde_close(s->fd);
+    qemu_free(s);
+}
+
 static int net_vde_init(VLANState *vlan, const char *model,
                         const char *name, const char *sock,
                         int port, const char *group, int mode)
@@ -1084,7 +1107,8 @@ static int net_vde_init(VLANState *vlan, const char *model,
         free(s);
         return -1;
     }
-    s->vc = qemu_new_vlan_client(vlan, model, name, vde_from_qemu, NULL, s);
+    s->vc = qemu_new_vlan_client(vlan, model, name, vde_from_qemu, NULL,
+                                 net_vde_cleanup, s);
     qemu_set_fd_handler(vde_datafd(s->vde), vde_to_qemu, NULL, s);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str), "sock=%s,fd=%d",
              sock, vde_datafd(s->vde));
@@ -1269,6 +1293,14 @@ fail:
     return -1;
 }
 
+static void net_socket_cleanup(void *opaque)
+{
+    NetSocketState *s = opaque;
+
+    close(s->fd);
+    qemu_free(s);
+}
+
 static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan,
                                                 const char *model,
                                                 const char *name,
@@ -1313,7 +1345,8 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan,
     s = qemu_mallocz(sizeof(NetSocketState));
     s->fd = fd;
 
-    s->vc = qemu_new_vlan_client(vlan, model, name, net_socket_receive_dgram, NULL, s);
+    s->vc = qemu_new_vlan_client(vlan, model, name, net_socket_receive_dgram,
+                                 NULL, net_socket_cleanup, s);
     qemu_set_fd_handler(s->fd, net_socket_send_dgram, NULL, s);
 
     /* mcast: save bound address as dst */
@@ -1340,8 +1373,8 @@ static NetSocketState *net_socket_fd_init_stream(VLANState *vlan,
     NetSocketState *s;
     s = qemu_mallocz(sizeof(NetSocketState));
     s->fd = fd;
-    s->vc = qemu_new_vlan_client(vlan, model, name,
-                                 net_socket_receive, NULL, s);
+    s->vc = qemu_new_vlan_client(vlan, model, name, net_socket_receive,
+                                 NULL, net_socket_cleanup, s);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str),
              "socket: fd=%d", fd);
     if (is_connected) {
@@ -1903,29 +1936,16 @@ done:
 
 void net_cleanup(void)
 {
-#if !defined(_WIN32)
     VLANState *vlan;
+    VLANClientState *vc;
 
-    /* close network clients */
-    for(vlan = first_vlan; vlan != NULL; vlan = vlan->next) {
-        VLANClientState *vc;
-
-        for(vc = vlan->first_client; vc != NULL; vc = vc->next) {
-            if (vc->fd_read == tap_receive) {
-                TAPState *s = vc->opaque;
-
-                if (s->down_script[0])
-                    launch_script(s->down_script, s->down_script_arg, s->fd);
-            }
-#if defined(CONFIG_VDE)
-            if (vc->fd_read == vde_from_qemu) {
-                VDEState *s = vc->opaque;
-                vde_close(s->vde);
+    for (vlan = first_vlan; vlan != NULL; vlan = vlan->next) {
+        for (vc = vlan->first_client; vc != NULL; vc = vc->next) {
+            if (vc->cleanup) {
+                vc->cleanup(vc->opaque);
             }
-#endif
         }
     }
-#endif
 }
 
 void net_client_check(void)
diff --git a/net.h b/net.h
index 1a51be7..571305b 100644
--- a/net.h
+++ b/net.h
@@ -11,7 +11,10 @@ typedef struct VLANClientState VLANClientState;
 
 typedef void (LinkStatusChanged)(VLANClientState *);
 
+typedef void (VLANClientCleanupHandler)(void *);
+
 struct VLANClientState {
+    VLANClientCleanupHandler *cleanup;
     IOReadHandler *fd_read;
     IOReadvHandler *fd_readv;
     /* Packets may still be sent if this returns zero.  It's used to
@@ -40,6 +43,7 @@ VLANClientState *qemu_new_vlan_client(VLANState *vlan,
                                       const char *name,
                                       IOReadHandler *fd_read,
                                       IOCanRWHandler *fd_can_read,
+                                      VLANClientCleanupHandler *cleanup,
                                       void *opaque);
 void qemu_del_vlan_client(VLANClientState *vc);
 VLANClientState *qemu_find_vlan_client(VLANState *vlan, void *opaque);

Re: [Qemu-devel] [PATCH 2/7] net: Add VLAN client cleanup handler

[Mark McLoughlin]

Hi Jan,

On Tue, 2009-04-14 at 19:29 +0200, Jan Kiszka wrote:
> Do proper VLAN client cleanup via a callback handler. This fixes
> resource leakage on host_net_remove and allows a generic net_cleanup
> implementation.

Yep, we need this.

However, I've got a different version in my queue (see below) - I've
been holding back on posting it until I finished some other patches I'm
working on, but I'll polish it off and post this afternoon.

Differences from yours include:

 - I've added cleanup code to all the NICs, including adding 
   unregister_savevm() so that we don't try and save deleted NICs

 - Rather than adding yet another param to new_vlan_client(), I just 
   initialize vc->cleanup after creating the client; another patch in
   my queue removes all callbacks to new_vlan_client() because as more
   are added it just gets terribly unwieldy.

 - I remove the io handler on e.g. the tapfd when freeing - otherwise 
   we'll continue to poll the fd AFAICS

 - I implement net_cleanup() by calling del_vlan_client() on all clients

Apart from that, the patches are very similar - are you okay with just
waiting for my patch?

Thanks,
Mark.

Re: [Qemu-devel] [PATCH 2/7] net: Add VLAN client cleanup handler

[Mark McLoughlin]

On Wed, 2009-04-15 at 14:18 +0100, Mark McLoughlin wrote:
> Hi Jan,
> 
> On Tue, 2009-04-14 at 19:29 +0200, Jan Kiszka wrote:
> > Do proper VLAN client cleanup via a callback handler. This fixes
> > resource leakage on host_net_remove and allows a generic net_cleanup
> > implementation.
> 
> Yep, we need this.
> 
> However, I've got a different version in my queue (see below) - I've
> been holding back on posting it until I finished some other patches I'm
> working on, but I'll polish it off and post this afternoon.
> 
> Differences from yours include:
> 
>  - I've added cleanup code to all the NICs, including adding 
>    unregister_savevm() so that we don't try and save deleted NICs
> 
>  - Rather than adding yet another param to new_vlan_client(), I just 
>    initialize vc->cleanup after creating the client; another patch in
>    my queue removes all callbacks to new_vlan_client() because as more
>    are added it just gets terribly unwieldy.
> 
>  - I remove the io handler on e.g. the tapfd when freeing - otherwise 
>    we'll continue to poll the fd AFAICS
> 
>  - I implement net_cleanup() by calling del_vlan_client() on all clients
> 
> Apart from that, the patches are very similar - are you okay with just
> waiting for my patch?

Doh, it would be helpful if I attached :-)

Cheers,
Mark.

From: Mark McLoughlin <markmc@redhat.com>
Subject: [PATCH] Introduce VLANClientState::cleanup()

We're currently leaking memory and file descriptors on device
hot-unplug.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 qemu/hw/e1000.c          |   12 +++++-----
 qemu/hw/eepro100.c       |   12 +++++++++
 qemu/hw/etraxfs_eth.c    |   11 +++++++++
 qemu/hw/mcf_fec.c        |   18 +++++++++++---
 qemu/hw/mipsnet.c        |   14 +++++++++++
 qemu/hw/musicpal.c       |   18 +++++++++++---
 qemu/hw/ne2000.c         |   24 +++++++++++++++++++
 qemu/hw/pcnet.c          |   43 +++++++++++++++++++++++++++++++---
 qemu/hw/rtl8139.c        |   20 ++++++++++++++++
 qemu/hw/smc91c111.c      |   17 ++++++++++---
 qemu/hw/stellaris_enet.c |   20 +++++++++++++---
 qemu/hw/usb-net.c        |   11 +++++++-
 qemu/hw/virtio-net.c     |   14 +++++++++++
 qemu/net.c               |   57 +++++++++++++++++++++++++++++++++------------
 qemu/net.h               |    2 +
 qemu/tap-win32.c         |   13 ++++++++++
 16 files changed, 263 insertions(+), 43 deletions(-)

diff --git a/qemu/hw/e1000.c b/qemu/hw/e1000.c
index 2d16774..978f789 100644
--- a/qemu/hw/e1000.c
+++ b/qemu/hw/e1000.c
@@ -1033,14 +1033,14 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
                                      excluded_regs[i] - 4);
 }
 
-static int
-pci_e1000_uninit(PCIDevice *dev)
+static void
+e1000_cleanup(VLANClientState *vc)
 {
-    E1000State *d = (E1000State *) dev;
+    E1000State *d = vc->opaque;
 
-    cpu_unregister_io_memory(d->mmio_index);
+    unregister_savevm("e1000", d);
 
-    return 0;
+    cpu_unregister_io_memory(d->mmio_index);
 }
 
 PCIDevice *
@@ -1095,12 +1095,12 @@ pci_e1000_init(PCIBus *bus, NICInfo *nd, int devfn)
 
     d->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  e1000_receive, e1000_can_receive, d);
+    d->vc->cleanup = e1000_cleanup;
     d->vc->link_status_changed = e1000_set_link_status;
 
     qemu_format_nic_info_str(d->vc, nd->macaddr);
 
     register_savevm(info_str, -1, 2, nic_save, nic_load, d);
-    d->dev.unregister = pci_e1000_uninit;
 
     return (PCIDevice *)d;
 }
diff --git a/qemu/hw/eepro100.c b/qemu/hw/eepro100.c
index 0a343df..dc50704 100644
--- a/qemu/hw/eepro100.c
+++ b/qemu/hw/eepro100.c
@@ -1710,6 +1710,17 @@ static void nic_save(QEMUFile * f, void *opaque)
     qemu_put_buffer(f, s->configuration, sizeof(s->configuration));
 }
 
+static void nic_cleanup(VLANClientState *vc)
+{
+    EEPRO100State *s = vc->opaque;
+
+    unregister_savevm(vc->name, s);
+
+    cpu_unregister_io_memory(s->mmio_index);
+
+    eeprom93xx_free(s->eeprom);
+}
+
 static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd,
                      const char *name, uint32_t device)
 {
@@ -1752,6 +1763,7 @@ static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd,
 
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  nic_receive, nic_can_receive, s);
+    s->vc->cleanup = nic_cleanup;
 
     qemu_format_nic_info_str(s->vc, s->macaddr);
 
diff --git a/qemu/hw/etraxfs_eth.c b/qemu/hw/etraxfs_eth.c
index c87e55f..b0f7f6d 100644
--- a/qemu/hw/etraxfs_eth.c
+++ b/qemu/hw/etraxfs_eth.c
@@ -554,6 +554,16 @@ static CPUWriteMemoryFunc *eth_write[] = {
 	&eth_writel,
 };
 
+static void etraxfs_eth_cleanup(VLANClientState *vc)
+{
+        struct fs_eth *eth = vc->opaque;
+
+        cpu_unregister_io_memory(eth->ethregs);
+
+        qemu_free(eth->dma_out);
+        qemu_free(eth);
+}
+
 void *etraxfs_eth_init(NICInfo *nd, CPUState *env, 
 		       qemu_irq *irq, target_phys_addr_t base, int phyaddr)
 {
@@ -586,6 +596,7 @@ void *etraxfs_eth_init(NICInfo *nd, CPUState *env,
 
 	eth->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
 				       eth_receive, eth_can_receive, eth);
+	eth->vc->cleanup = etraxfs_eth_cleanup;
 	eth->vc->opaque = eth;
 	eth->vc->link_status_changed = eth_set_link;
 
diff --git a/qemu/hw/mcf_fec.c b/qemu/hw/mcf_fec.c
index 413c569..6204772 100644
--- a/qemu/hw/mcf_fec.c
+++ b/qemu/hw/mcf_fec.c
@@ -24,6 +24,7 @@ do { printf("mcf_fec: " fmt , ##args); } while (0)
 
 typedef struct {
     qemu_irq *irq;
+    int mmio_index;
     VLANClientState *vc;
     uint32_t irq_state;
     uint32_t eir;
@@ -441,21 +442,30 @@ static CPUWriteMemoryFunc *mcf_fec_writefn[] = {
    mcf_fec_write
 };
 
+static void mcf_fec_cleanup(VLANClientState *vc)
+{
+    mcf_fec_state *s = vc->opaque;
+
+    cpu_unregister_io_memory(s->mmio_index);
+
+    qemu_free(s);
+}
+
 void mcf_fec_init(NICInfo *nd, target_phys_addr_t base, qemu_irq *irq)
 {
     mcf_fec_state *s;
-    int iomemtype;
 
     qemu_check_nic_model(nd, "mcf_fec");
 
     s = (mcf_fec_state *)qemu_mallocz(sizeof(mcf_fec_state));
     s->irq = irq;
-    iomemtype = cpu_register_io_memory(0, mcf_fec_readfn,
-                                       mcf_fec_writefn, s);
-    cpu_register_physical_memory(base, 0x400, iomemtype);
+    s->mmio_index = cpu_register_io_memory(0, mcf_fec_readfn,
+                                           mcf_fec_writefn, s);
+    cpu_register_physical_memory(base, 0x400, s->mmio_index);
 
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  mcf_fec_receive, mcf_fec_can_receive, s);
+    s->vc->cleanup = mcf_fec_cleanup;
     memcpy(s->macaddr, nd->macaddr, 6);
     qemu_format_nic_info_str(s->vc, s->macaddr);
 }
diff --git a/qemu/hw/mipsnet.c b/qemu/hw/mipsnet.c
index de001ec..1fde386 100644
--- a/qemu/hw/mipsnet.c
+++ b/qemu/hw/mipsnet.c
@@ -33,6 +33,7 @@ typedef struct MIPSnetState {
     uint32_t intctl;
     uint8_t rx_buffer[MAX_ETH_FRAME_SIZE];
     uint8_t tx_buffer[MAX_ETH_FRAME_SIZE];
+    int io_base;
     qemu_irq irq;
     VLANClientState *vc;
 } MIPSnetState;
@@ -231,6 +232,17 @@ static int mipsnet_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+static void mipsnet_cleanup(VLANClientState *vc)
+{
+    MIPSnetState *s = vc->opaque;
+
+    unregister_savevm("mipsnet", s);
+
+    isa_unassign_ioport(s->io_base, 36);
+
+    qemu_free(s);
+}
+
 void mipsnet_init (int base, qemu_irq irq, NICInfo *nd)
 {
     MIPSnetState *s;
@@ -246,9 +258,11 @@ void mipsnet_init (int base, qemu_irq irq, NICInfo *nd)
     register_ioport_write(base, 36, 4, mipsnet_ioport_write, s);
     register_ioport_read(base, 36, 4, mipsnet_ioport_read, s);
 
+    s->io_base = base;
     s->irq = irq;
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  mipsnet_receive, mipsnet_can_receive, s);
+    s->vc->cleanup = mipsnet_cleanup;
 
     qemu_format_nic_info_str(s->vc, nd->macaddr);
 
diff --git a/qemu/hw/musicpal.c b/qemu/hw/musicpal.c
index b19115f..395d2ea 100644
--- a/qemu/hw/musicpal.c
+++ b/qemu/hw/musicpal.c
@@ -554,6 +554,7 @@ typedef struct mv88w8618_eth_state {
     uint32_t smir;
     uint32_t icr;
     uint32_t imr;
+    int mmio_index;
     int vlan_header;
     mv88w8618_tx_desc *tx_queue[2];
     mv88w8618_rx_desc *rx_queue[4];
@@ -714,10 +715,18 @@ static CPUWriteMemoryFunc *mv88w8618_eth_writefn[] = {
     mv88w8618_eth_write
 };
 
+static void mv88w8618_eth_cleanup(VLANClientState *vc)
+{
+    mv88w8618_eth_state *s = vc->opaque;
+
+    cpu_unregister_io_memory(s->mmio_index);
+
+    qemu_free(s);
+}
+
 static void mv88w8618_eth_init(NICInfo *nd, uint32_t base, qemu_irq irq)
 {
     mv88w8618_eth_state *s;
-    int iomemtype;
 
     qemu_check_nic_model(nd, "mv88w8618");
 
@@ -725,9 +734,10 @@ static void mv88w8618_eth_init(NICInfo *nd, uint32_t base, qemu_irq irq)
     s->irq = irq;
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  eth_receive, eth_can_receive, s);
-    iomemtype = cpu_register_io_memory(0, mv88w8618_eth_readfn,
-                                       mv88w8618_eth_writefn, s);
-    cpu_register_physical_memory(base, MP_ETH_SIZE, iomemtype);
+    s->vc->cleanup = mv88w8618_eth_cleanup;
+    s->mmio_index = cpu_register_io_memory(0, mv88w8618_eth_readfn,
+                                           mv88w8618_eth_writefn, s);
+    cpu_register_physical_memory(base, MP_ETH_SIZE, s->mmio_index);
 }
 
 /* LCD register offsets */
diff --git a/qemu/hw/ne2000.c b/qemu/hw/ne2000.c
index 24a66bb..7fe3975 100644
--- a/qemu/hw/ne2000.c
+++ b/qemu/hw/ne2000.c
@@ -140,6 +140,7 @@ typedef struct NE2000State {
     uint8_t curpag;
     uint8_t mult[8]; /* multicast mask array */
     qemu_irq irq;
+    int isa_io_base;
     PCIDevice *pci_dev;
     VLANClientState *vc;
     uint8_t macaddr[6];
@@ -718,6 +719,19 @@ static int ne2000_load(QEMUFile* f,void* opaque,int version_id)
 	return 0;
 }
 
+static void isa_ne2000_cleanup(VLANClientState *vc)
+{
+    NE2000State *s = vc->opaque;
+
+    unregister_savevm("ne2000", s);
+
+    isa_unassign_ioport(s->isa_io_base, 16);
+    isa_unassign_ioport(s->isa_io_base + 0x10, 2);
+    isa_unassign_ioport(s->isa_io_base + 0x1f, 1);
+
+    qemu_free(s);
+}
+
 void isa_ne2000_init(int base, qemu_irq irq, NICInfo *nd)
 {
     NE2000State *s;
@@ -736,6 +750,7 @@ void isa_ne2000_init(int base, qemu_irq irq, NICInfo *nd)
 
     register_ioport_write(base + 0x1f, 1, 1, ne2000_reset_ioport_write, s);
     register_ioport_read(base + 0x1f, 1, 1, ne2000_reset_ioport_read, s);
+    s->isa_io_base = base;
     s->irq = irq;
     memcpy(s->macaddr, nd->macaddr, 6);
 
@@ -743,6 +758,7 @@ void isa_ne2000_init(int base, qemu_irq irq, NICInfo *nd)
 
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  ne2000_receive, ne2000_can_receive, s);
+    s->vc->cleanup = isa_ne2000_cleanup;
 
     qemu_format_nic_info_str(s->vc, s->macaddr);
 
@@ -777,6 +793,13 @@ static void ne2000_map(PCIDevice *pci_dev, int region_num,
     register_ioport_read(addr + 0x1f, 1, 1, ne2000_reset_ioport_read, s);
 }
 
+static void ne2000_cleanup(VLANClientState *vc)
+{
+    NE2000State *s = vc->opaque;
+
+    unregister_savevm("ne2000", s);
+}
+
 PCIDevice *pci_ne2000_init(PCIBus *bus, NICInfo *nd, int devfn)
 {
     PCINE2000State *d;
@@ -803,6 +826,7 @@ PCIDevice *pci_ne2000_init(PCIBus *bus, NICInfo *nd, int devfn)
     ne2000_reset(s);
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  ne2000_receive, ne2000_can_receive, s);
+    s->vc->cleanup = ne2000_cleanup;
 
     qemu_format_nic_info_str(s->vc, s->macaddr);
 
diff --git a/qemu/hw/pcnet.c b/qemu/hw/pcnet.c
index 0e63c3d..dd22a6c 100644
--- a/qemu/hw/pcnet.c
+++ b/qemu/hw/pcnet.c
@@ -75,6 +75,7 @@ struct PCNetState_st {
     uint8_t buffer[4096];
     int tx_busy;
     qemu_irq irq;
+    qemu_irq *reset_irq;
     void (*phys_mem_read)(void *dma_opaque, target_phys_addr_t addr,
                          uint8_t *buf, int len, int do_bswap);
     void (*phys_mem_write)(void *dma_opaque, target_phys_addr_t addr,
@@ -1929,6 +1930,13 @@ static int pcnet_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+static void pcnet_common_cleanup(PCNetState *d)
+{
+    unregister_savevm("pcnet", d);
+    qemu_del_timer(d->poll_timer);
+    qemu_free_timer(d->poll_timer);
+}
+
 static void pcnet_common_init(PCNetState *d, NICInfo *nd)
 {
     d->poll_timer = qemu_new_timer(vm_clock, pcnet_poll_timer, d);
@@ -1982,6 +1990,15 @@ static void pci_physical_memory_read(void *dma_opaque, target_phys_addr_t addr,
     cpu_physical_memory_read(addr, buf, len);
 }
 
+static void pci_pcnet_cleanup(NetClient *client)
+{
+    PCNetState *d = client->opaque;
+
+    pcnet_common_cleanup(d);
+
+    cpu_unregister_io_memory(d->mmio_index);
+}
+
 PCIDevice *pci_pcnet_init(PCIBus *bus, NICInfo *nd, int devfn)
 {
     PCNetState *d;
@@ -2029,6 +2046,9 @@ PCIDevice *pci_pcnet_init(PCIBus *bus, NICInfo *nd, int devfn)
     d->pci_dev = &d->dev;
 
     pcnet_common_init(d, nd);
+
+    d->client->cleanup = pci_pcnet_cleanup;
+
     return (PCIDevice *)d;
 }
 
@@ -2078,29 +2098,44 @@ static CPUWriteMemoryFunc *lance_mem_write[3] = {
     NULL,
 };
 
+static void lance_cleanup(NetClient *client)
+{
+    PCNetState *d = client->opaque;
+
+    pcnet_common_cleanup(d);
+
+    qemu_free_irqs(d->reset_irq);
+
+    cpu_unregister_io_memory(d->mmio_index);
+
+    qemu_free(d);
+}
+
 void lance_init(NICInfo *nd, target_phys_addr_t leaddr, void *dma_opaque,
                 qemu_irq irq, qemu_irq *reset)
 {
     PCNetState *d;
-    int lance_io_memory;
 
     qemu_check_nic_model(nd, "lance");
 
     d = qemu_mallocz(sizeof(PCNetState));
 
-    lance_io_memory =
+    d->mmio_index =
         cpu_register_io_memory(0, lance_mem_read, lance_mem_write, d);
 
     d->dma_opaque = dma_opaque;
 
-    *reset = *qemu_allocate_irqs(parent_lance_reset, d, 1);
+    d->reset_irq = qemu_allocate_irqs(parent_lance_reset, d, 1);
+    *reset = *d->reset_irq;
 
-    cpu_register_physical_memory(leaddr, 4, lance_io_memory);
+    cpu_register_physical_memory(leaddr, 4, d->mmio_index);
 
     d->irq = irq;
     d->phys_mem_read = ledma_memory_read;
     d->phys_mem_write = ledma_memory_write;
 
     pcnet_common_init(d, nd);
+
+    d->client->cleanup = lance_cleanup;
 }
 #endif /* TARGET_SPARC */
diff --git a/qemu/hw/rtl8139.c b/qemu/hw/rtl8139.c
index 9fa69db..e381ab0 100644
--- a/qemu/hw/rtl8139.c
+++ b/qemu/hw/rtl8139.c
@@ -3414,6 +3414,25 @@ static void rtl8139_timer(void *opaque)
 }
 #endif /* RTL8139_ONBOARD_TIMER */
 
+static void rtl8139_cleanup(VLANClientState *vc)
+{
+    RTL8139State *s = vc->opaque;
+
+    if (s->cplus_txbuffer) {
+        qemu_free(s->cplus_txbuffer);
+        s->cplus_txbuffer = NULL;
+    }
+
+#ifdef RTL8139_ONBOARD_TIMER
+    qemu_del_timer(s->timer);
+    qemu_free_timer(s->timer);
+#endif
+
+    unregister_savevm("rtl8139", s);
+
+    cpu_unregister_io_memory(s->rtl8139_mmio_io_addr);
+}
+
 PCIDevice *pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
 {
     PCIRTL8139State *d;
@@ -3451,6 +3470,7 @@ PCIDevice *pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
     rtl8139_reset(s);
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  rtl8139_receive, rtl8139_can_receive, s);
+    s->vc->cleanup = rtl8139_cleanup;
 
     qemu_format_nic_info_str(s->vc, s->macaddr);
 
diff --git a/qemu/hw/smc91c111.c b/qemu/hw/smc91c111.c
index f5b29a7..b132ca3 100644
--- a/qemu/hw/smc91c111.c
+++ b/qemu/hw/smc91c111.c
@@ -42,6 +42,7 @@ typedef struct {
     uint8_t int_level;
     uint8_t int_mask;
     uint8_t macaddr[6];
+    int mmio_index;
 } smc91c111_state;
 
 #define RCR_SOFT_RST  0x8000
@@ -690,17 +691,24 @@ static CPUWriteMemoryFunc *smc91c111_writefn[] = {
     smc91c111_writel
 };
 
+static void smc91c111_cleanup(VLANClientState *vc)
+{
+    smc91c111_state *s = vc->opaque;
+
+    cpu_unregister_io_memory(s->mmio_index);
+    qemu_free(s);
+}
+
 void smc91c111_init(NICInfo *nd, uint32_t base, qemu_irq irq)
 {
     smc91c111_state *s;
-    int iomemtype;
 
     qemu_check_nic_model(nd, "smc91c111");
 
     s = (smc91c111_state *)qemu_mallocz(sizeof(smc91c111_state));
-    iomemtype = cpu_register_io_memory(0, smc91c111_readfn,
-                                       smc91c111_writefn, s);
-    cpu_register_physical_memory(base, 16, iomemtype);
+    s->mmio_index = cpu_register_io_memory(0, smc91c111_readfn,
+                                           smc91c111_writefn, s);
+    cpu_register_physical_memory(base, 16, s->mmio_index);
     s->irq = irq;
     memcpy(s->macaddr, nd->macaddr, 6);
 
@@ -708,6 +716,7 @@ void smc91c111_init(NICInfo *nd, uint32_t base, qemu_irq irq)
 
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  smc91c111_receive, smc91c111_can_receive, s);
+    s->vc->cleanup = smc91c111_cleanup;
     qemu_format_nic_info_str(s->vc, s->macaddr);
     /* ??? Save/restore.  */
 }
diff --git a/qemu/hw/stellaris_enet.c b/qemu/hw/stellaris_enet.c
index 9d39169..a029607 100644
--- a/qemu/hw/stellaris_enet.c
+++ b/qemu/hw/stellaris_enet.c
@@ -69,6 +69,7 @@ typedef struct {
     VLANClientState *vc;
     qemu_irq irq;
     uint8_t macaddr[6];
+    int mmio_index;
 } stellaris_enet_state;
 
 static void stellaris_enet_update(stellaris_enet_state *s)
@@ -384,22 +385,33 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+static void stellaris_enet_cleanup(VLANClientState *vc)
+{
+    stellaris_enet_state *s = vc->opaque;
+
+    unregister_savevm("stellaris_enet", s);
+
+    cpu_unregister_io_memory(s->mmio_index);
+
+    qemu_free(s);
+}
+
 void stellaris_enet_init(NICInfo *nd, uint32_t base, qemu_irq irq)
 {
     stellaris_enet_state *s;
-    int iomemtype;
 
     qemu_check_nic_model(nd, "stellaris");
 
     s = (stellaris_enet_state *)qemu_mallocz(sizeof(stellaris_enet_state));
-    iomemtype = cpu_register_io_memory(0, stellaris_enet_readfn,
-                                       stellaris_enet_writefn, s);
-    cpu_register_physical_memory(base, 0x00001000, iomemtype);
+    s->mmio_index = cpu_register_io_memory(0, stellaris_enet_readfn,
+                                           stellaris_enet_writefn, s);
+    cpu_register_physical_memory(base, 0x00001000, s->mmio_index);
     s->irq = irq;
     memcpy(s->macaddr, nd->macaddr, 6);
 
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  stellaris_enet_receive, stellaris_enet_can_receive, s);
+    s->vc->cleanup = stellaris_enet_cleanup;
     qemu_format_nic_info_str(s->vc, s->macaddr);
 
     stellaris_enet_reset(s);
diff --git a/qemu/hw/usb-net.c b/qemu/hw/usb-net.c
index 863c25f..99c132f 100644
--- a/qemu/hw/usb-net.c
+++ b/qemu/hw/usb-net.c
@@ -1415,14 +1415,20 @@ static int usbnet_can_receive(void *opaque)
     return !s->in_len;
 }
 
+static void usbnet_cleanup(VLANClientState *vc)
+{
+    USBNetState *s = vc->opaque;
+
+    rndis_clear_responsequeue(s);
+    qemu_free(s);
+}
+
 static void usb_net_handle_destroy(USBDevice *dev)
 {
     USBNetState *s = (USBNetState *) dev;
 
     /* TODO: remove the nd_table[] entry */
     qemu_del_vlan_client(s->vc);
-    rndis_clear_responsequeue(s);
-    qemu_free(s);
 }
 
 USBDevice *usb_net_init(NICInfo *nd)
@@ -1453,6 +1459,7 @@ USBDevice *usb_net_init(NICInfo *nd)
                     "QEMU USB Network Interface");
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                     usbnet_receive, usbnet_can_receive, s);
+    s->vc->cleanup = usbnet_cleanup;
 
     qemu_format_nic_info_str(s->vc, s->mac);
 
diff --git a/qemu/hw/virtio-net.c b/qemu/hw/virtio-net.c
index 094a2fb..650cc76 100644
--- a/qemu/hw/virtio-net.c
+++ b/qemu/hw/virtio-net.c
@@ -570,6 +570,19 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+static void virtio_net_cleanup(VLANClientState *vc)
+{
+    VirtIONet *n = vc->opaque;
+
+    unregister_savevm("virtio-net", n);
+
+    qemu_free(n->mac_table.macs);
+    qemu_free(n->vlans);
+
+    qemu_del_timer(n->tx_timer);
+    qemu_free_timer(n->tx_timer);
+}
+
 PCIDevice *virtio_net_init(PCIBus *bus, NICInfo *nd, int devfn)
 {
     VirtIONet *n;
@@ -599,6 +612,7 @@ PCIDevice *virtio_net_init(PCIBus *bus, NICInfo *nd, int devfn)
     n->status = VIRTIO_NET_S_LINK_UP;
     n->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  virtio_net_receive, virtio_net_can_receive, n);
+    n->vc->cleanup = virtio_net_cleanup;
     n->vc->link_status_changed = virtio_net_set_link_status;
 
     qemu_format_nic_info_str(n->vc, n->mac);
diff --git a/qemu/net.c b/qemu/net.c
index e3abe89..2d1c1b4 100644
--- a/qemu/net.c
+++ b/qemu/net.c
@@ -362,6 +362,8 @@ void qemu_del_vlan_client(VLANClientState *vc)
     while (*pvc != NULL)
         if (*pvc == vc) {
             *pvc = vc->next;
+            if (vc->cleanup)
+                vc->cleanup(client);
             free(vc->name);
             free(vc->model);
             free(vc);
@@ -709,6 +711,8 @@ typedef struct TAPState {
     char down_script_arg[128];
 } TAPState;
 
+static int launch_script(const char *setup_script, const char *ifname, int fd);
+
 static ssize_t tap_receive_iov(void *opaque, const struct iovec *iov,
                                int iovcnt)
 {
@@ -775,6 +779,18 @@ static void tap_send(void *opaque)
     }
 }
 
+static void tap_cleanup(VLANClientState *vc)
+{
+    TAPState *s = vc->opaque;
+
+    if (s->down_script[0])
+        launch_script(s->down_script, s->down_script_arg, s->fd);
+
+    qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+    close(s->fd);
+    free(s);
+}
+
 /* fd support */
 
 static TAPState *net_tap_fd_init(VLANState *vlan,
@@ -787,6 +803,7 @@ static TAPState *net_tap_fd_init(VLANState *vlan,
     s = qemu_mallocz(sizeof(TAPState));
     s->fd = fd;
     s->vc = qemu_new_vlan_client(vlan, model, name, tap_receive, NULL, s);
+    s->vc->cleanup = tap_cleanup;
     s->vc->fd_readv = tap_receive_iov;
     qemu_set_fd_handler2(s->fd, tap_can_send, tap_send, NULL, s);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str), "fd=%d", fd);
@@ -1085,6 +1102,14 @@ static void vde_from_qemu(void *opaque, const uint8_t *buf, int size)
     }
 }
 
+static void vde_cleanup(VLANClientState *vc)
+{
+    VDEState *s = vc->opaque;
+    qemu_set_fd_handler(vde_datafd(s->vde), NULL, NULL, NULL);
+    vde_close(s->vde);
+    free(s);
+}
+
 static int net_vde_init(VLANState *vlan, const char *model,
                         const char *name, const char *sock,
                         int port, const char *group, int mode)
@@ -1106,6 +1131,7 @@ static int net_vde_init(VLANState *vlan, const char *model,
         return -1;
     }
     s->vc = qemu_new_vlan_client(vlan, model, name, vde_from_qemu, NULL, s);
+    s->vc->cleanup = vde_cleanup;
     qemu_set_fd_handler(vde_datafd(s->vde), vde_to_qemu, NULL, s);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str), "sock=%s,fd=%d",
              sock, vde_datafd(s->vde));
@@ -1290,6 +1316,14 @@ fail:
     return -1;
 }
 
+static void net_socket_cleanup(VLANClientState *vc)
+{
+    NetSocketState *s = vc->opaque;
+    qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+    close(s->fd);
+    free(s);
+}
+
 static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan,
                                                 const char *model,
                                                 const char *name,
@@ -1335,6 +1369,7 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan,
     s->fd = fd;
 
     s->vc = qemu_new_vlan_client(vlan, model, name, net_socket_receive_dgram, NULL, s);
+    s->vc->cleanup = net_socket_cleanup;
     qemu_set_fd_handler(s->fd, net_socket_send_dgram, NULL, s);
 
     /* mcast: save bound address as dst */
@@ -1363,6 +1398,7 @@ static NetSocketState *net_socket_fd_init_stream(VLANState *vlan,
     s->fd = fd;
     s->vc = qemu_new_vlan_client(vlan, model, name,
                                  net_socket_receive, NULL, s);
+    s->vc->cleanup = net_socket_cleanup;
     snprintf(s->vc->info_str, sizeof(s->vc->info_str),
              "socket: fd=%d", fd);
     if (is_connected) {
@@ -1922,29 +1958,20 @@ done:
 
 void net_cleanup(void)
 {
-#if !defined(_WIN32)
     VLANState *vlan;
 
     /* close network clients */
     for(vlan = first_vlan; vlan != NULL; vlan = vlan->next) {
-        VLANClientState *vc;
+        VLANClientState *vc = vlan->first_client;
 
-        for(vc = vlan->first_client; vc != NULL; vc = vc->next) {
-            if (vc->fd_read == tap_receive) {
-                TAPState *s = vc->opaque;
+        while (vc) {
+            VLANClientState *next = vc->next;
 
-                if (s->down_script[0])
-                    launch_script(s->down_script, s->down_script_arg, s->fd);
-            }
-#if defined(CONFIG_VDE)
-            if (vc->fd_read == vde_from_qemu) {
-                VDEState *s = vc->opaque;
-                vde_close(s->vde);
-            }
-#endif
+            qemu_del_vlan_client(vc);
+
+            vc = next;
         }
     }
-#endif
 }
 
 void net_client_check(void)
diff --git a/qemu/net.h b/qemu/net.h
index 1a51be7..5def263 100644
--- a/qemu/net.h
+++ b/qemu/net.h
@@ -9,6 +9,7 @@ typedef ssize_t (IOReadvHandler)(void *, const struct iovec *, int);
 
 typedef struct VLANClientState VLANClientState;
 
+typedef void (NetCleanup) (VLANClientState *);
 typedef void (LinkStatusChanged)(VLANClientState *);
 
 struct VLANClientState {
@@ -17,6 +18,7 @@ struct VLANClientState {
     /* Packets may still be sent if this returns zero.  It's used to
        rate-limit the slirp code.  */
     IOCanRWHandler *fd_can_read;
+    NetCleanup *cleanup;
     LinkStatusChanged *link_status_changed;
     int link_down;
     void *opaque;
diff --git a/qemu/tap-win32.c b/qemu/tap-win32.c
index e8a04dc..5948060 100644
--- a/qemu/tap-win32.c
+++ b/qemu/tap-win32.c
@@ -638,6 +638,18 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
      tap_win32_overlapped_t *handle;
  } TAPState;
 
+static void tap_cleanup(VLANClientState *vc)
+{
+    TAPState *s = vc->opaque;
+
+    qemu_del_wait_object(s->handle->tap_semaphore, NULL, NULL);
+
+    /* FIXME: need to kill thread and close file handle:
+       tap_win32_close(s);
+    */
+    free(s);
+}
+
 static void tap_receive(void *opaque, const uint8_t *buf, int size)
 {
     TAPState *s = opaque;
@@ -673,6 +685,7 @@ int tap_win32_init(VLANState *vlan, const char *model,
     }
 
     s->vc = qemu_new_vlan_client(vlan, model, name, tap_receive, NULL, s);
+    s->vc->cleanup = tap_cleanup;
 
     snprintf(s->vc->info_str, sizeof(s->vc->info_str),
              "tap: ifname=%s", ifname);
-- 
1.6.0.6

[Qemu-devel] Re: [PATCH 2/7] net: Add VLAN client cleanup handler

[Jan Kiszka]

Mark McLoughlin wrote:
> On Wed, 2009-04-15 at 14:18 +0100, Mark McLoughlin wrote:
>> Hi Jan,
>>
>> On Tue, 2009-04-14 at 19:29 +0200, Jan Kiszka wrote:
>>> Do proper VLAN client cleanup via a callback handler. This fixes
>>> resource leakage on host_net_remove and allows a generic net_cleanup
>>> implementation.
>> Yep, we need this.
>>
>> However, I've got a different version in my queue (see below) - I've
>> been holding back on posting it until I finished some other patches I'm
>> working on, but I'll polish it off and post this afternoon.
>>
>> Differences from yours include:
>>
>>  - I've added cleanup code to all the NICs, including adding 
>>    unregister_savevm() so that we don't try and save deleted NICs
>>
>>  - Rather than adding yet another param to new_vlan_client(), I just 
>>    initialize vc->cleanup after creating the client; another patch in
>>    my queue removes all callbacks to new_vlan_client() because as more
>>    are added it just gets terribly unwieldy.
>>
>>  - I remove the io handler on e.g. the tapfd when freeing - otherwise 
>>    we'll continue to poll the fd AFAICS
>>
>>  - I implement net_cleanup() by calling del_vlan_client() on all clients
>>
>> Apart from that, the patches are very similar - are you okay with just
>> waiting for my patch?
> 
> Doh, it would be helpful if I attached :-)

Ah, good, there it is. Is it considered ready for inclusion, also into
stable? Then I would start rebasing my patches and give feedback if I
happen to find quirks.

Jan

> 
> Cheers,
> Mark.
> 
> From: Mark McLoughlin <markmc@redhat.com>
> Subject: [PATCH] Introduce VLANClientState::cleanup()
> 
> We're currently leaking memory and file descriptors on device
> hot-unplug.
> 
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> ---
>  qemu/hw/e1000.c          |   12 +++++-----
>  qemu/hw/eepro100.c       |   12 +++++++++
>  qemu/hw/etraxfs_eth.c    |   11 +++++++++
>  qemu/hw/mcf_fec.c        |   18 +++++++++++---
>  qemu/hw/mipsnet.c        |   14 +++++++++++
>  qemu/hw/musicpal.c       |   18 +++++++++++---
>  qemu/hw/ne2000.c         |   24 +++++++++++++++++++
>  qemu/hw/pcnet.c          |   43 +++++++++++++++++++++++++++++++---
>  qemu/hw/rtl8139.c        |   20 ++++++++++++++++
>  qemu/hw/smc91c111.c      |   17 ++++++++++---
>  qemu/hw/stellaris_enet.c |   20 +++++++++++++---
>  qemu/hw/usb-net.c        |   11 +++++++-
>  qemu/hw/virtio-net.c     |   14 +++++++++++
>  qemu/net.c               |   57 +++++++++++++++++++++++++++++++++------------
>  qemu/net.h               |    2 +
>  qemu/tap-win32.c         |   13 ++++++++++
>  16 files changed, 263 insertions(+), 43 deletions(-)
> 
> diff --git a/qemu/hw/e1000.c b/qemu/hw/e1000.c
> index 2d16774..978f789 100644
> --- a/qemu/hw/e1000.c
> +++ b/qemu/hw/e1000.c
> @@ -1033,14 +1033,14 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
>                                       excluded_regs[i] - 4);
>  }
>  
> -static int
> -pci_e1000_uninit(PCIDevice *dev)
> +static void
> +e1000_cleanup(VLANClientState *vc)
>  {
> -    E1000State *d = (E1000State *) dev;
> +    E1000State *d = vc->opaque;
>  
> -    cpu_unregister_io_memory(d->mmio_index);
> +    unregister_savevm("e1000", d);
>  
> -    return 0;
> +    cpu_unregister_io_memory(d->mmio_index);
>  }
>  
>  PCIDevice *
> @@ -1095,12 +1095,12 @@ pci_e1000_init(PCIBus *bus, NICInfo *nd, int devfn)
>  
>      d->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                                   e1000_receive, e1000_can_receive, d);
> +    d->vc->cleanup = e1000_cleanup;
>      d->vc->link_status_changed = e1000_set_link_status;
>  
>      qemu_format_nic_info_str(d->vc, nd->macaddr);
>  
>      register_savevm(info_str, -1, 2, nic_save, nic_load, d);
> -    d->dev.unregister = pci_e1000_uninit;
>  
>      return (PCIDevice *)d;
>  }
> diff --git a/qemu/hw/eepro100.c b/qemu/hw/eepro100.c
> index 0a343df..dc50704 100644
> --- a/qemu/hw/eepro100.c
> +++ b/qemu/hw/eepro100.c
> @@ -1710,6 +1710,17 @@ static void nic_save(QEMUFile * f, void *opaque)
>      qemu_put_buffer(f, s->configuration, sizeof(s->configuration));
>  }
>  
> +static void nic_cleanup(VLANClientState *vc)
> +{
> +    EEPRO100State *s = vc->opaque;
> +
> +    unregister_savevm(vc->name, s);
> +
> +    cpu_unregister_io_memory(s->mmio_index);
> +
> +    eeprom93xx_free(s->eeprom);
> +}
> +
>  static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd,
>                       const char *name, uint32_t device)
>  {
> @@ -1752,6 +1763,7 @@ static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd,
>  
>      s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                                   nic_receive, nic_can_receive, s);
> +    s->vc->cleanup = nic_cleanup;
>  
>      qemu_format_nic_info_str(s->vc, s->macaddr);
>  
> diff --git a/qemu/hw/etraxfs_eth.c b/qemu/hw/etraxfs_eth.c
> index c87e55f..b0f7f6d 100644
> --- a/qemu/hw/etraxfs_eth.c
> +++ b/qemu/hw/etraxfs_eth.c
> @@ -554,6 +554,16 @@ static CPUWriteMemoryFunc *eth_write[] = {
>  	&eth_writel,
>  };
>  
> +static void etraxfs_eth_cleanup(VLANClientState *vc)
> +{
> +        struct fs_eth *eth = vc->opaque;
> +
> +        cpu_unregister_io_memory(eth->ethregs);
> +
> +        qemu_free(eth->dma_out);
> +        qemu_free(eth);
> +}
> +
>  void *etraxfs_eth_init(NICInfo *nd, CPUState *env, 
>  		       qemu_irq *irq, target_phys_addr_t base, int phyaddr)
>  {
> @@ -586,6 +596,7 @@ void *etraxfs_eth_init(NICInfo *nd, CPUState *env,
>  
>  	eth->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>  				       eth_receive, eth_can_receive, eth);
> +	eth->vc->cleanup = etraxfs_eth_cleanup;
>  	eth->vc->opaque = eth;
>  	eth->vc->link_status_changed = eth_set_link;
>  
> diff --git a/qemu/hw/mcf_fec.c b/qemu/hw/mcf_fec.c
> index 413c569..6204772 100644
> --- a/qemu/hw/mcf_fec.c
> +++ b/qemu/hw/mcf_fec.c
> @@ -24,6 +24,7 @@ do { printf("mcf_fec: " fmt , ##args); } while (0)
>  
>  typedef struct {
>      qemu_irq *irq;
> +    int mmio_index;
>      VLANClientState *vc;
>      uint32_t irq_state;
>      uint32_t eir;
> @@ -441,21 +442,30 @@ static CPUWriteMemoryFunc *mcf_fec_writefn[] = {
>     mcf_fec_write
>  };
>  
> +static void mcf_fec_cleanup(VLANClientState *vc)
> +{
> +    mcf_fec_state *s = vc->opaque;
> +
> +    cpu_unregister_io_memory(s->mmio_index);
> +
> +    qemu_free(s);
> +}
> +
>  void mcf_fec_init(NICInfo *nd, target_phys_addr_t base, qemu_irq *irq)
>  {
>      mcf_fec_state *s;
> -    int iomemtype;
>  
>      qemu_check_nic_model(nd, "mcf_fec");
>  
>      s = (mcf_fec_state *)qemu_mallocz(sizeof(mcf_fec_state));
>      s->irq = irq;
> -    iomemtype = cpu_register_io_memory(0, mcf_fec_readfn,
> -                                       mcf_fec_writefn, s);
> -    cpu_register_physical_memory(base, 0x400, iomemtype);
> +    s->mmio_index = cpu_register_io_memory(0, mcf_fec_readfn,
> +                                           mcf_fec_writefn, s);
> +    cpu_register_physical_memory(base, 0x400, s->mmio_index);
>  
>      s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                                   mcf_fec_receive, mcf_fec_can_receive, s);
> +    s->vc->cleanup = mcf_fec_cleanup;
>      memcpy(s->macaddr, nd->macaddr, 6);
>      qemu_format_nic_info_str(s->vc, s->macaddr);
>  }
> diff --git a/qemu/hw/mipsnet.c b/qemu/hw/mipsnet.c
> index de001ec..1fde386 100644
> --- a/qemu/hw/mipsnet.c
> +++ b/qemu/hw/mipsnet.c
> @@ -33,6 +33,7 @@ typedef struct MIPSnetState {
>      uint32_t intctl;
>      uint8_t rx_buffer[MAX_ETH_FRAME_SIZE];
>      uint8_t tx_buffer[MAX_ETH_FRAME_SIZE];
> +    int io_base;
>      qemu_irq irq;
>      VLANClientState *vc;
>  } MIPSnetState;
> @@ -231,6 +232,17 @@ static int mipsnet_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +static void mipsnet_cleanup(VLANClientState *vc)
> +{
> +    MIPSnetState *s = vc->opaque;
> +
> +    unregister_savevm("mipsnet", s);
> +
> +    isa_unassign_ioport(s->io_base, 36);
> +
> +    qemu_free(s);
> +}
> +
>  void mipsnet_init (int base, qemu_irq irq, NICInfo *nd)
>  {
>      MIPSnetState *s;
> @@ -246,9 +258,11 @@ void mipsnet_init (int base, qemu_irq irq, NICInfo *nd)
>      register_ioport_write(base, 36, 4, mipsnet_ioport_write, s);
>      register_ioport_read(base, 36, 4, mipsnet_ioport_read, s);
>  
> +    s->io_base = base;
>      s->irq = irq;
>      s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                                   mipsnet_receive, mipsnet_can_receive, s);
> +    s->vc->cleanup = mipsnet_cleanup;
>  
>      qemu_format_nic_info_str(s->vc, nd->macaddr);
>  
> diff --git a/qemu/hw/musicpal.c b/qemu/hw/musicpal.c
> index b19115f..395d2ea 100644
> --- a/qemu/hw/musicpal.c
> +++ b/qemu/hw/musicpal.c
> @@ -554,6 +554,7 @@ typedef struct mv88w8618_eth_state {
>      uint32_t smir;
>      uint32_t icr;
>      uint32_t imr;
> +    int mmio_index;
>      int vlan_header;
>      mv88w8618_tx_desc *tx_queue[2];
>      mv88w8618_rx_desc *rx_queue[4];
> @@ -714,10 +715,18 @@ static CPUWriteMemoryFunc *mv88w8618_eth_writefn[] = {
>      mv88w8618_eth_write
>  };
>  
> +static void mv88w8618_eth_cleanup(VLANClientState *vc)
> +{
> +    mv88w8618_eth_state *s = vc->opaque;
> +
> +    cpu_unregister_io_memory(s->mmio_index);
> +
> +    qemu_free(s);
> +}
> +
>  static void mv88w8618_eth_init(NICInfo *nd, uint32_t base, qemu_irq irq)
>  {
>      mv88w8618_eth_state *s;
> -    int iomemtype;
>  
>      qemu_check_nic_model(nd, "mv88w8618");
>  
> @@ -725,9 +734,10 @@ static void mv88w8618_eth_init(NICInfo *nd, uint32_t base, qemu_irq irq)
>      s->irq = irq;
>      s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                                   eth_receive, eth_can_receive, s);
> -    iomemtype = cpu_register_io_memory(0, mv88w8618_eth_readfn,
> -                                       mv88w8618_eth_writefn, s);
> -    cpu_register_physical_memory(base, MP_ETH_SIZE, iomemtype);
> +    s->vc->cleanup = mv88w8618_eth_cleanup;
> +    s->mmio_index = cpu_register_io_memory(0, mv88w8618_eth_readfn,
> +                                           mv88w8618_eth_writefn, s);
> +    cpu_register_physical_memory(base, MP_ETH_SIZE, s->mmio_index);
>  }
>  
>  /* LCD register offsets */
> diff --git a/qemu/hw/ne2000.c b/qemu/hw/ne2000.c
> index 24a66bb..7fe3975 100644
> --- a/qemu/hw/ne2000.c
> +++ b/qemu/hw/ne2000.c
> @@ -140,6 +140,7 @@ typedef struct NE2000State {
>      uint8_t curpag;
>      uint8_t mult[8]; /* multicast mask array */
>      qemu_irq irq;
> +    int isa_io_base;
>      PCIDevice *pci_dev;
>      VLANClientState *vc;
>      uint8_t macaddr[6];
> @@ -718,6 +719,19 @@ static int ne2000_load(QEMUFile* f,void* opaque,int version_id)
>  	return 0;
>  }
>  
> +static void isa_ne2000_cleanup(VLANClientState *vc)
> +{
> +    NE2000State *s = vc->opaque;
> +
> +    unregister_savevm("ne2000", s);
> +
> +    isa_unassign_ioport(s->isa_io_base, 16);
> +    isa_unassign_ioport(s->isa_io_base + 0x10, 2);
> +    isa_unassign_ioport(s->isa_io_base + 0x1f, 1);
> +
> +    qemu_free(s);
> +}
> +
>  void isa_ne2000_init(int base, qemu_irq irq, NICInfo *nd)
>  {
>      NE2000State *s;
> @@ -736,6 +750,7 @@ void isa_ne2000_init(int base, qemu_irq irq, NICInfo *nd)
>  
>      register_ioport_write(base + 0x1f, 1, 1, ne2000_reset_ioport_write, s);
>      register_ioport_read(base + 0x1f, 1, 1, ne2000_reset_ioport_read, s);
> +    s->isa_io_base = base;
>      s->irq = irq;
>      memcpy(s->macaddr, nd->macaddr, 6);
>  
> @@ -743,6 +758,7 @@ void isa_ne2000_init(int base, qemu_irq irq, NICInfo *nd)
>  
>      s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                                   ne2000_receive, ne2000_can_receive, s);
> +    s->vc->cleanup = isa_ne2000_cleanup;
>  
>      qemu_format_nic_info_str(s->vc, s->macaddr);
>  
> @@ -777,6 +793,13 @@ static void ne2000_map(PCIDevice *pci_dev, int region_num,
>      register_ioport_read(addr + 0x1f, 1, 1, ne2000_reset_ioport_read, s);
>  }
>  
> +static void ne2000_cleanup(VLANClientState *vc)
> +{
> +    NE2000State *s = vc->opaque;
> +
> +    unregister_savevm("ne2000", s);
> +}
> +
>  PCIDevice *pci_ne2000_init(PCIBus *bus, NICInfo *nd, int devfn)
>  {
>      PCINE2000State *d;
> @@ -803,6 +826,7 @@ PCIDevice *pci_ne2000_init(PCIBus *bus, NICInfo *nd, int devfn)
>      ne2000_reset(s);
>      s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                                   ne2000_receive, ne2000_can_receive, s);
> +    s->vc->cleanup = ne2000_cleanup;
>  
>      qemu_format_nic_info_str(s->vc, s->macaddr);
>  
> diff --git a/qemu/hw/pcnet.c b/qemu/hw/pcnet.c
> index 0e63c3d..dd22a6c 100644
> --- a/qemu/hw/pcnet.c
> +++ b/qemu/hw/pcnet.c
> @@ -75,6 +75,7 @@ struct PCNetState_st {
>      uint8_t buffer[4096];
>      int tx_busy;
>      qemu_irq irq;
> +    qemu_irq *reset_irq;
>      void (*phys_mem_read)(void *dma_opaque, target_phys_addr_t addr,
>                           uint8_t *buf, int len, int do_bswap);
>      void (*phys_mem_write)(void *dma_opaque, target_phys_addr_t addr,
> @@ -1929,6 +1930,13 @@ static int pcnet_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +static void pcnet_common_cleanup(PCNetState *d)
> +{
> +    unregister_savevm("pcnet", d);
> +    qemu_del_timer(d->poll_timer);
> +    qemu_free_timer(d->poll_timer);
> +}
> +
>  static void pcnet_common_init(PCNetState *d, NICInfo *nd)
>  {
>      d->poll_timer = qemu_new_timer(vm_clock, pcnet_poll_timer, d);
> @@ -1982,6 +1990,15 @@ static void pci_physical_memory_read(void *dma_opaque, target_phys_addr_t addr,
>      cpu_physical_memory_read(addr, buf, len);
>  }
>  
> +static void pci_pcnet_cleanup(NetClient *client)
> +{
> +    PCNetState *d = client->opaque;
> +
> +    pcnet_common_cleanup(d);
> +
> +    cpu_unregister_io_memory(d->mmio_index);
> +}
> +
>  PCIDevice *pci_pcnet_init(PCIBus *bus, NICInfo *nd, int devfn)
>  {
>      PCNetState *d;
> @@ -2029,6 +2046,9 @@ PCIDevice *pci_pcnet_init(PCIBus *bus, NICInfo *nd, int devfn)
>      d->pci_dev = &d->dev;
>  
>      pcnet_common_init(d, nd);
> +
> +    d->client->cleanup = pci_pcnet_cleanup;
> +
>      return (PCIDevice *)d;
>  }
>  
> @@ -2078,29 +2098,44 @@ static CPUWriteMemoryFunc *lance_mem_write[3] = {
>      NULL,
>  };
>  
> +static void lance_cleanup(NetClient *client)
> +{
> +    PCNetState *d = client->opaque;
> +
> +    pcnet_common_cleanup(d);
> +
> +    qemu_free_irqs(d->reset_irq);
> +
> +    cpu_unregister_io_memory(d->mmio_index);
> +
> +    qemu_free(d);
> +}
> +
>  void lance_init(NICInfo *nd, target_phys_addr_t leaddr, void *dma_opaque,
>                  qemu_irq irq, qemu_irq *reset)
>  {
>      PCNetState *d;
> -    int lance_io_memory;
>  
>      qemu_check_nic_model(nd, "lance");
>  
>      d = qemu_mallocz(sizeof(PCNetState));
>  
> -    lance_io_memory =
> +    d->mmio_index =
>          cpu_register_io_memory(0, lance_mem_read, lance_mem_write, d);
>  
>      d->dma_opaque = dma_opaque;
>  
> -    *reset = *qemu_allocate_irqs(parent_lance_reset, d, 1);
> +    d->reset_irq = qemu_allocate_irqs(parent_lance_reset, d, 1);
> +    *reset = *d->reset_irq;
>  
> -    cpu_register_physical_memory(leaddr, 4, lance_io_memory);
> +    cpu_register_physical_memory(leaddr, 4, d->mmio_index);
>  
>      d->irq = irq;
>      d->phys_mem_read = ledma_memory_read;
>      d->phys_mem_write = ledma_memory_write;
>  
>      pcnet_common_init(d, nd);
> +
> +    d->client->cleanup = lance_cleanup;
>  }
>  #endif /* TARGET_SPARC */
> diff --git a/qemu/hw/rtl8139.c b/qemu/hw/rtl8139.c
> index 9fa69db..e381ab0 100644
> --- a/qemu/hw/rtl8139.c
> +++ b/qemu/hw/rtl8139.c
> @@ -3414,6 +3414,25 @@ static void rtl8139_timer(void *opaque)
>  }
>  #endif /* RTL8139_ONBOARD_TIMER */
>  
> +static void rtl8139_cleanup(VLANClientState *vc)
> +{
> +    RTL8139State *s = vc->opaque;
> +
> +    if (s->cplus_txbuffer) {
> +        qemu_free(s->cplus_txbuffer);
> +        s->cplus_txbuffer = NULL;
> +    }
> +
> +#ifdef RTL8139_ONBOARD_TIMER
> +    qemu_del_timer(s->timer);
> +    qemu_free_timer(s->timer);
> +#endif
> +
> +    unregister_savevm("rtl8139", s);
> +
> +    cpu_unregister_io_memory(s->rtl8139_mmio_io_addr);
> +}
> +
>  PCIDevice *pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
>  {
>      PCIRTL8139State *d;
> @@ -3451,6 +3470,7 @@ PCIDevice *pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
>      rtl8139_reset(s);
>      s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                                   rtl8139_receive, rtl8139_can_receive, s);
> +    s->vc->cleanup = rtl8139_cleanup;
>  
>      qemu_format_nic_info_str(s->vc, s->macaddr);
>  
> diff --git a/qemu/hw/smc91c111.c b/qemu/hw/smc91c111.c
> index f5b29a7..b132ca3 100644
> --- a/qemu/hw/smc91c111.c
> +++ b/qemu/hw/smc91c111.c
> @@ -42,6 +42,7 @@ typedef struct {
>      uint8_t int_level;
>      uint8_t int_mask;
>      uint8_t macaddr[6];
> +    int mmio_index;
>  } smc91c111_state;
>  
>  #define RCR_SOFT_RST  0x8000
> @@ -690,17 +691,24 @@ static CPUWriteMemoryFunc *smc91c111_writefn[] = {
>      smc91c111_writel
>  };
>  
> +static void smc91c111_cleanup(VLANClientState *vc)
> +{
> +    smc91c111_state *s = vc->opaque;
> +
> +    cpu_unregister_io_memory(s->mmio_index);
> +    qemu_free(s);
> +}
> +
>  void smc91c111_init(NICInfo *nd, uint32_t base, qemu_irq irq)
>  {
>      smc91c111_state *s;
> -    int iomemtype;
>  
>      qemu_check_nic_model(nd, "smc91c111");
>  
>      s = (smc91c111_state *)qemu_mallocz(sizeof(smc91c111_state));
> -    iomemtype = cpu_register_io_memory(0, smc91c111_readfn,
> -                                       smc91c111_writefn, s);
> -    cpu_register_physical_memory(base, 16, iomemtype);
> +    s->mmio_index = cpu_register_io_memory(0, smc91c111_readfn,
> +                                           smc91c111_writefn, s);
> +    cpu_register_physical_memory(base, 16, s->mmio_index);
>      s->irq = irq;
>      memcpy(s->macaddr, nd->macaddr, 6);
>  
> @@ -708,6 +716,7 @@ void smc91c111_init(NICInfo *nd, uint32_t base, qemu_irq irq)
>  
>      s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                                   smc91c111_receive, smc91c111_can_receive, s);
> +    s->vc->cleanup = smc91c111_cleanup;
>      qemu_format_nic_info_str(s->vc, s->macaddr);
>      /* ??? Save/restore.  */
>  }
> diff --git a/qemu/hw/stellaris_enet.c b/qemu/hw/stellaris_enet.c
> index 9d39169..a029607 100644
> --- a/qemu/hw/stellaris_enet.c
> +++ b/qemu/hw/stellaris_enet.c
> @@ -69,6 +69,7 @@ typedef struct {
>      VLANClientState *vc;
>      qemu_irq irq;
>      uint8_t macaddr[6];
> +    int mmio_index;
>  } stellaris_enet_state;
>  
>  static void stellaris_enet_update(stellaris_enet_state *s)
> @@ -384,22 +385,33 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +static void stellaris_enet_cleanup(VLANClientState *vc)
> +{
> +    stellaris_enet_state *s = vc->opaque;
> +
> +    unregister_savevm("stellaris_enet", s);
> +
> +    cpu_unregister_io_memory(s->mmio_index);
> +
> +    qemu_free(s);
> +}
> +
>  void stellaris_enet_init(NICInfo *nd, uint32_t base, qemu_irq irq)
>  {
>      stellaris_enet_state *s;
> -    int iomemtype;
>  
>      qemu_check_nic_model(nd, "stellaris");
>  
>      s = (stellaris_enet_state *)qemu_mallocz(sizeof(stellaris_enet_state));
> -    iomemtype = cpu_register_io_memory(0, stellaris_enet_readfn,
> -                                       stellaris_enet_writefn, s);
> -    cpu_register_physical_memory(base, 0x00001000, iomemtype);
> +    s->mmio_index = cpu_register_io_memory(0, stellaris_enet_readfn,
> +                                           stellaris_enet_writefn, s);
> +    cpu_register_physical_memory(base, 0x00001000, s->mmio_index);
>      s->irq = irq;
>      memcpy(s->macaddr, nd->macaddr, 6);
>  
>      s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                                   stellaris_enet_receive, stellaris_enet_can_receive, s);
> +    s->vc->cleanup = stellaris_enet_cleanup;
>      qemu_format_nic_info_str(s->vc, s->macaddr);
>  
>      stellaris_enet_reset(s);
> diff --git a/qemu/hw/usb-net.c b/qemu/hw/usb-net.c
> index 863c25f..99c132f 100644
> --- a/qemu/hw/usb-net.c
> +++ b/qemu/hw/usb-net.c
> @@ -1415,14 +1415,20 @@ static int usbnet_can_receive(void *opaque)
>      return !s->in_len;
>  }
>  
> +static void usbnet_cleanup(VLANClientState *vc)
> +{
> +    USBNetState *s = vc->opaque;
> +
> +    rndis_clear_responsequeue(s);
> +    qemu_free(s);
> +}
> +
>  static void usb_net_handle_destroy(USBDevice *dev)
>  {
>      USBNetState *s = (USBNetState *) dev;
>  
>      /* TODO: remove the nd_table[] entry */
>      qemu_del_vlan_client(s->vc);
> -    rndis_clear_responsequeue(s);
> -    qemu_free(s);
>  }
>  
>  USBDevice *usb_net_init(NICInfo *nd)
> @@ -1453,6 +1459,7 @@ USBDevice *usb_net_init(NICInfo *nd)
>                      "QEMU USB Network Interface");
>      s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                      usbnet_receive, usbnet_can_receive, s);
> +    s->vc->cleanup = usbnet_cleanup;
>  
>      qemu_format_nic_info_str(s->vc, s->mac);
>  
> diff --git a/qemu/hw/virtio-net.c b/qemu/hw/virtio-net.c
> index 094a2fb..650cc76 100644
> --- a/qemu/hw/virtio-net.c
> +++ b/qemu/hw/virtio-net.c
> @@ -570,6 +570,19 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +static void virtio_net_cleanup(VLANClientState *vc)
> +{
> +    VirtIONet *n = vc->opaque;
> +
> +    unregister_savevm("virtio-net", n);
> +
> +    qemu_free(n->mac_table.macs);
> +    qemu_free(n->vlans);
> +
> +    qemu_del_timer(n->tx_timer);
> +    qemu_free_timer(n->tx_timer);
> +}
> +
>  PCIDevice *virtio_net_init(PCIBus *bus, NICInfo *nd, int devfn)
>  {
>      VirtIONet *n;
> @@ -599,6 +612,7 @@ PCIDevice *virtio_net_init(PCIBus *bus, NICInfo *nd, int devfn)
>      n->status = VIRTIO_NET_S_LINK_UP;
>      n->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                                   virtio_net_receive, virtio_net_can_receive, n);
> +    n->vc->cleanup = virtio_net_cleanup;
>      n->vc->link_status_changed = virtio_net_set_link_status;
>  
>      qemu_format_nic_info_str(n->vc, n->mac);
> diff --git a/qemu/net.c b/qemu/net.c
> index e3abe89..2d1c1b4 100644
> --- a/qemu/net.c
> +++ b/qemu/net.c
> @@ -362,6 +362,8 @@ void qemu_del_vlan_client(VLANClientState *vc)
>      while (*pvc != NULL)
>          if (*pvc == vc) {
>              *pvc = vc->next;
> +            if (vc->cleanup)
> +                vc->cleanup(client);
>              free(vc->name);
>              free(vc->model);
>              free(vc);
> @@ -709,6 +711,8 @@ typedef struct TAPState {
>      char down_script_arg[128];
>  } TAPState;
>  
> +static int launch_script(const char *setup_script, const char *ifname, int fd);
> +
>  static ssize_t tap_receive_iov(void *opaque, const struct iovec *iov,
>                                 int iovcnt)
>  {
> @@ -775,6 +779,18 @@ static void tap_send(void *opaque)
>      }
>  }
>  
> +static void tap_cleanup(VLANClientState *vc)
> +{
> +    TAPState *s = vc->opaque;
> +
> +    if (s->down_script[0])
> +        launch_script(s->down_script, s->down_script_arg, s->fd);
> +
> +    qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
> +    close(s->fd);
> +    free(s);
> +}
> +
>  /* fd support */
>  
>  static TAPState *net_tap_fd_init(VLANState *vlan,
> @@ -787,6 +803,7 @@ static TAPState *net_tap_fd_init(VLANState *vlan,
>      s = qemu_mallocz(sizeof(TAPState));
>      s->fd = fd;
>      s->vc = qemu_new_vlan_client(vlan, model, name, tap_receive, NULL, s);
> +    s->vc->cleanup = tap_cleanup;
>      s->vc->fd_readv = tap_receive_iov;
>      qemu_set_fd_handler2(s->fd, tap_can_send, tap_send, NULL, s);
>      snprintf(s->vc->info_str, sizeof(s->vc->info_str), "fd=%d", fd);
> @@ -1085,6 +1102,14 @@ static void vde_from_qemu(void *opaque, const uint8_t *buf, int size)
>      }
>  }
>  
> +static void vde_cleanup(VLANClientState *vc)
> +{
> +    VDEState *s = vc->opaque;
> +    qemu_set_fd_handler(vde_datafd(s->vde), NULL, NULL, NULL);
> +    vde_close(s->vde);
> +    free(s);
> +}
> +
>  static int net_vde_init(VLANState *vlan, const char *model,
>                          const char *name, const char *sock,
>                          int port, const char *group, int mode)
> @@ -1106,6 +1131,7 @@ static int net_vde_init(VLANState *vlan, const char *model,
>          return -1;
>      }
>      s->vc = qemu_new_vlan_client(vlan, model, name, vde_from_qemu, NULL, s);
> +    s->vc->cleanup = vde_cleanup;
>      qemu_set_fd_handler(vde_datafd(s->vde), vde_to_qemu, NULL, s);
>      snprintf(s->vc->info_str, sizeof(s->vc->info_str), "sock=%s,fd=%d",
>               sock, vde_datafd(s->vde));
> @@ -1290,6 +1316,14 @@ fail:
>      return -1;
>  }
>  
> +static void net_socket_cleanup(VLANClientState *vc)
> +{
> +    NetSocketState *s = vc->opaque;
> +    qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
> +    close(s->fd);
> +    free(s);
> +}
> +
>  static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan,
>                                                  const char *model,
>                                                  const char *name,
> @@ -1335,6 +1369,7 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan,
>      s->fd = fd;
>  
>      s->vc = qemu_new_vlan_client(vlan, model, name, net_socket_receive_dgram, NULL, s);
> +    s->vc->cleanup = net_socket_cleanup;
>      qemu_set_fd_handler(s->fd, net_socket_send_dgram, NULL, s);
>  
>      /* mcast: save bound address as dst */
> @@ -1363,6 +1398,7 @@ static NetSocketState *net_socket_fd_init_stream(VLANState *vlan,
>      s->fd = fd;
>      s->vc = qemu_new_vlan_client(vlan, model, name,
>                                   net_socket_receive, NULL, s);
> +    s->vc->cleanup = net_socket_cleanup;
>      snprintf(s->vc->info_str, sizeof(s->vc->info_str),
>               "socket: fd=%d", fd);
>      if (is_connected) {
> @@ -1922,29 +1958,20 @@ done:
>  
>  void net_cleanup(void)
>  {
> -#if !defined(_WIN32)
>      VLANState *vlan;
>  
>      /* close network clients */
>      for(vlan = first_vlan; vlan != NULL; vlan = vlan->next) {
> -        VLANClientState *vc;
> +        VLANClientState *vc = vlan->first_client;
>  
> -        for(vc = vlan->first_client; vc != NULL; vc = vc->next) {
> -            if (vc->fd_read == tap_receive) {
> -                TAPState *s = vc->opaque;
> +        while (vc) {
> +            VLANClientState *next = vc->next;
>  
> -                if (s->down_script[0])
> -                    launch_script(s->down_script, s->down_script_arg, s->fd);
> -            }
> -#if defined(CONFIG_VDE)
> -            if (vc->fd_read == vde_from_qemu) {
> -                VDEState *s = vc->opaque;
> -                vde_close(s->vde);
> -            }
> -#endif
> +            qemu_del_vlan_client(vc);
> +
> +            vc = next;
>          }
>      }
> -#endif
>  }
>  
>  void net_client_check(void)
> diff --git a/qemu/net.h b/qemu/net.h
> index 1a51be7..5def263 100644
> --- a/qemu/net.h
> +++ b/qemu/net.h
> @@ -9,6 +9,7 @@ typedef ssize_t (IOReadvHandler)(void *, const struct iovec *, int);
>  
>  typedef struct VLANClientState VLANClientState;
>  
> +typedef void (NetCleanup) (VLANClientState *);
>  typedef void (LinkStatusChanged)(VLANClientState *);
>  
>  struct VLANClientState {
> @@ -17,6 +18,7 @@ struct VLANClientState {
>      /* Packets may still be sent if this returns zero.  It's used to
>         rate-limit the slirp code.  */
>      IOCanRWHandler *fd_can_read;
> +    NetCleanup *cleanup;
>      LinkStatusChanged *link_status_changed;
>      int link_down;
>      void *opaque;
> diff --git a/qemu/tap-win32.c b/qemu/tap-win32.c
> index e8a04dc..5948060 100644
> --- a/qemu/tap-win32.c
> +++ b/qemu/tap-win32.c
> @@ -638,6 +638,18 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
>       tap_win32_overlapped_t *handle;
>   } TAPState;
>  
> +static void tap_cleanup(VLANClientState *vc)
> +{
> +    TAPState *s = vc->opaque;
> +
> +    qemu_del_wait_object(s->handle->tap_semaphore, NULL, NULL);
> +
> +    /* FIXME: need to kill thread and close file handle:
> +       tap_win32_close(s);
> +    */
> +    free(s);
> +}
> +
>  static void tap_receive(void *opaque, const uint8_t *buf, int size)
>  {
>      TAPState *s = opaque;
> @@ -673,6 +685,7 @@ int tap_win32_init(VLANState *vlan, const char *model,
>      }
>  
>      s->vc = qemu_new_vlan_client(vlan, model, name, tap_receive, NULL, s);
> +    s->vc->cleanup = tap_cleanup;
>  
>      snprintf(s->vc->info_str, sizeof(s->vc->info_str),
>               "tap: ifname=%s", ifname);

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 2/7] net: Add VLAN client cleanup handler

[Mark McLoughlin]

On Wed, 2009-04-15 at 16:13 +0200, Jan Kiszka wrote:

> Ah, good, there it is. Is it considered ready for inclusion, also into
> stable? Then I would start rebasing my patches and give feedback if I
> happen to find quirks.

Thanks, I'd greatly appreciate you giving it a bash.

As for pushing to stable - it's a fairly big patch, with the potential
to introduce crashers. I'd be wary of applying it to stable, but I guess
it all depends on how cautious we're being about stable.

Cheers,
Mark.

[Qemu-devel] Re: [PATCH 2/7] net: Add VLAN client cleanup handler

[Jan Kiszka]

Mark McLoughlin wrote:
> Hi Jan,
> 
> On Tue, 2009-04-14 at 19:29 +0200, Jan Kiszka wrote:
>> Do proper VLAN client cleanup via a callback handler. This fixes
>> resource leakage on host_net_remove and allows a generic net_cleanup
>> implementation.
> 
> Yep, we need this.
> 
> However, I've got a different version in my queue (see below) - I've
> been holding back on posting it until I finished some other patches I'm
> working on, but I'll polish it off and post this afternoon.
> 
> Differences from yours include:
> 
>  - I've added cleanup code to all the NICs, including adding 
>    unregister_savevm() so that we don't try and save deleted NICs

Different issue than I was trying to fix (host net removals), but valid.

> 
>  - Rather than adding yet another param to new_vlan_client(), I just 
>    initialize vc->cleanup after creating the client; another patch in
>    my queue removes all callbacks to new_vlan_client() because as more
>    are added it just gets terribly unwieldy.

Personally, I prefer a function-based API over, well, hacking the
structures directly. The driver should not have to poke into its device
descriptor. Yes, this breaks existing code each time you add another
callback, but this has the effect that you
 a) normally think twice if you really need it before doing this,
 b) likely enhance all users, and
 c) that new users will find prominent information about this
    (hopefully) useful callback.

> 
>  - I remove the io handler on e.g. the tapfd when freeing - otherwise 
>    we'll continue to poll the fd AFAICS

Good point. I ran into similar issues lately with the qemu-char code
(fixed upstream now).

> 
>  - I implement net_cleanup() by calling del_vlan_client() on all clients

Yes, probably the better way around.

> 
> Apart from that, the patches are very similar - are you okay with just
> waiting for my patch?

If we can agree on second point :) and if your patch will show up soon,
I will happily rebase the rest of my series on top of it.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 2/7] net: Add VLAN client cleanup handler

[Mark McLoughlin]

On Wed, 2009-04-15 at 16:04 +0200, Jan Kiszka wrote:
> Mark McLoughlin wrote:
>  
> >  - Rather than adding yet another param to new_vlan_client(), I just 
> >    initialize vc->cleanup after creating the client; another patch in
> >    my queue removes all callbacks to new_vlan_client() because as more
> >    are added it just gets terribly unwieldy.
> 
> Personally, I prefer a function-based API over, well, hacking the
> structures directly. The driver should not have to poke into its device
> descriptor.

I see this as similar to filing out a vtable. Perhaps it makes sense to
split the function pointers out into a separate struct.

>  Yes, this breaks existing code each time you add another
> callback, but this has the effect that you
>  a) normally think twice if you really need it before doing this,
>  b) likely enhance all users, and
>  c) that new users will find prominent information about this
>     (hopefully) useful callback.

The way I see it is that often when you're adding a new callback, you
don't want to require all users to implement it. Instead you make sure
there is a reasonable default behaviour for when the callback is NULL,
and only set it to non-NULL for the users that really need it.

Cheers,
Mark.

[Qemu-devel] Re: [PATCH 2/7] net: Add VLAN client cleanup handler

[Jan Kiszka]

Mark McLoughlin wrote:
> On Wed, 2009-04-15 at 16:04 +0200, Jan Kiszka wrote:
>> Mark McLoughlin wrote:
>>  
>>>  - Rather than adding yet another param to new_vlan_client(), I just 
>>>    initialize vc->cleanup after creating the client; another patch in
>>>    my queue removes all callbacks to new_vlan_client() because as more
>>>    are added it just gets terribly unwieldy.
>> Personally, I prefer a function-based API over, well, hacking the
>> structures directly. The driver should not have to poke into its device
>> descriptor.
> 
> I see this as similar to filing out a vtable. Perhaps it makes sense to
> split the function pointers out into a separate struct.
> 
>>  Yes, this breaks existing code each time you add another
>> callback, but this has the effect that you
>>  a) normally think twice if you really need it before doing this,
>>  b) likely enhance all users, and
>>  c) that new users will find prominent information about this
>>     (hopefully) useful callback.
> 
> The way I see it is that often when you're adding a new callback, you
> don't want to require all users to implement it. Instead you make sure
> there is a reasonable default behaviour for when the callback is NULL,
> and only set it to non-NULL for the users that really need it.

Based on our patches, one could easily argue that the cleanup handler
does not fall into this category - nearly everyone wants it, no?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 1/7] net: Fix -net socket,listen

[Jan Kiszka]

In case no symbolic name is provided when requesting VLAN connection via
listening TCP socket ('-net socket,listen=...'), qemu crashes. This
fixes the cause.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 net.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net.c b/net.c
index 5365891..c7e4a0b 100644
--- a/net.c
+++ b/net.c
@@ -1440,7 +1440,7 @@ static int net_socket_listen_init(VLANState *vlan,
     }
     s->vlan = vlan;
     s->model = strdup(model);
-    s->name = strdup(name);
+    s->name = name ? strdup(name) : NULL;
     s->fd = fd;
     qemu_set_fd_handler(fd, net_socket_accept, NULL, s);
     return 0;

Re: [Qemu-devel] [PATCH 1/7] net: Fix -net socket,listen

[Mark McLoughlin]

On Tue, 2009-04-14 at 19:29 +0200, Jan Kiszka wrote:
> In case no symbolic name is provided when requesting VLAN connection via
> listening TCP socket ('-net socket,listen=...'), qemu crashes. This
> fixes the cause.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  net.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net.c b/net.c
> index 5365891..c7e4a0b 100644
> --- a/net.c
> +++ b/net.c
> @@ -1440,7 +1440,7 @@ static int net_socket_listen_init(VLANState *vlan,
>      }
>      s->vlan = vlan;
>      s->model = strdup(model);
> -    s->name = strdup(name);
> +    s->name = name ? strdup(name) : NULL;

Yep, this is a slightly unusual case because we're not allocating a
VLANClientState here which would cause a default name to be assigned.

Acked-by: Mark McLoughlin <markmc@redhat.com>

Cheers,
Mark.

[Qemu-devel] [PATCH 4/7] net: Prevent multiple slirp instances

[Jan Kiszka]

The slirp stack is full of global variables which prevents instantiating
it more than once. Catch this during net_slirp_init to prevent more harm
later on.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 net.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/net.c b/net.c
index 787f249..0486f7c 100644
--- a/net.c
+++ b/net.c
@@ -519,15 +519,27 @@ static void slirp_receive(void *opaque, const uint8_t *buf, int size)
     slirp_input(buf, size);
 }
 
+static int slirp_in_use;
+
+static void net_slirp_cleanup(void *opaque)
+{
+    slirp_in_use = 0;
+}
+
 static int net_slirp_init(VLANState *vlan, const char *model, const char *name)
 {
+    if (slirp_in_use) {
+        /* slirp only supports a single instance so far */
+        return -1;
+    }
     if (!slirp_inited) {
         slirp_inited = 1;
         slirp_init(slirp_restrict, slirp_ip);
     }
     slirp_vc = qemu_new_vlan_client(vlan, model, name,
-                                    slirp_receive, NULL, NULL, NULL);
+                                    slirp_receive, NULL, net_slirp_cleanup, NULL);
     slirp_vc->info_str[0] = '\0';
+    slirp_in_use = 1;
     return 0;
 }
 

Re: [Qemu-devel] [PATCH 4/7] net: Prevent multiple slirp instances

[Mark McLoughlin]

On Tue, 2009-04-14 at 19:29 +0200, Jan Kiszka wrote:
> The slirp stack is full of global variables which prevents instantiating
> it more than once. Catch this during net_slirp_init to prevent more harm
> later on.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  net.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/net.c b/net.c
> index 787f249..0486f7c 100644
> --- a/net.c
> +++ b/net.c
> @@ -519,15 +519,27 @@ static void slirp_receive(void *opaque, const uint8_t *buf, int size)
>      slirp_input(buf, size);
>  }
>  
> +static int slirp_in_use;
> +
> +static void net_slirp_cleanup(void *opaque)
> +{
> +    slirp_in_use = 0;
> +}
> +
>  static int net_slirp_init(VLANState *vlan, const char *model, const char *name)
>  {
> +    if (slirp_in_use) {
> +        /* slirp only supports a single instance so far */
> +        return -1;
> +    }
>      if (!slirp_inited) {
>          slirp_inited = 1;
>          slirp_init(slirp_restrict, slirp_ip);
>      }
>      slirp_vc = qemu_new_vlan_client(vlan, model, name,
> -                                    slirp_receive, NULL, NULL, NULL);
> +                                    slirp_receive, NULL, net_slirp_cleanup, NULL);
>      slirp_vc->info_str[0] = '\0';
> +    slirp_in_use = 1;
>      return 0;

Yeah, it's ugly but probably the best we can do right now.

Acked-by: Mark McLoughlin <markmc@redhat.com>

Cheers,
Mark.

[Qemu-devel] [PATCH 6/7] net: Add support for capturing VLANs

[Jan Kiszka]

This patch is derived from Tristan Gingold's patch. It adds a new VLAN
client type that writes all traffic on the VLAN it is attached to into a
pcap file. Such a file can then be analyzed offline with Wireshark or
tcpdump.

Besides rebasing and some minor cleanups, the major differences to the
original version are:
 - support for enabling/disabling via the monitor (host_net_add/remove)
 - always register dump client at the head of a VLAN queue
   (instead of special handling for slirp)
 - 64k default capturing limit (I hate tcpdump's default)

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 monitor.c       |    2 -
 net.c           |  163 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 net.h           |    7 ++
 qemu-options.hx |    7 ++
 4 files changed, 168 insertions(+), 11 deletions(-)

diff --git a/monitor.c b/monitor.c
index d9f84a7..dc2008a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1731,7 +1731,7 @@ static const mon_cmd_t mon_cmds[] = {
     { "pci_add", "sss", pci_device_hot_add, "pci_addr=auto|[[<domain>:]<bus>:]<slot> nic|storage [[vlan=n][,macaddr=addr][,model=type]] [file=file][,if=type][,bus=nr]...", "hot-add PCI device" },
     { "pci_del", "s", pci_device_hot_remove, "pci_addr=[[<domain>:]<bus>:]<slot>", "hot remove PCI device" },
     { "host_net_add", "ss?", net_host_device_add,
-      "tap|user|socket|vde [options]", "add host VLAN client" },
+      "tap|user|socket|vde|dump [options]", "add host VLAN client" },
     { "host_net_remove", "is", net_host_device_remove,
       "vlan_id name", "remove host VLAN client" },
 #endif
diff --git a/net.c b/net.c
index e812eb8..d9e986e 100644
--- a/net.c
+++ b/net.c
@@ -118,6 +118,7 @@
 #include "qemu-char.h"
 #include "audio/audio.h"
 #include "qemu_socket.h"
+#include "qemu-log.h"
 
 #if defined(CONFIG_SLIRP)
 #include "libslirp.h"
@@ -328,15 +329,16 @@ static char *assign_name(VLANClientState *vc1, const char *model)
     return strdup(buf);
 }
 
-VLANClientState *qemu_new_vlan_client(VLANState *vlan,
-                                      const char *model,
-                                      const char *name,
-                                      IOReadHandler *fd_read,
-                                      IOCanRWHandler *fd_can_read,
-                                      VLANClientCleanupHandler *cleanup,
-                                      void *opaque)
+static VLANClientState *new_vlan_client(VLANState *vlan,
+                                        const char *model,
+                                        const char *name,
+                                        IOReadHandler *fd_read,
+                                        IOCanRWHandler *fd_can_read,
+                                        VLANClientCleanupHandler *cleanup,
+                                        void *opaque)
 {
-    VLANClientState *vc, **pvc;
+    VLANClientState *vc;
+
     vc = qemu_mallocz(sizeof(VLANClientState));
     vc->model = strdup(model);
     if (name)
@@ -348,6 +350,20 @@ VLANClientState *qemu_new_vlan_client(VLANState *vlan,
     vc->cleanup = cleanup;
     vc->opaque = opaque;
     vc->vlan = vlan;
+    return vc;
+}
+
+VLANClientState *qemu_new_vlan_client(VLANState *vlan,
+                                      const char *model,
+                                      const char *name,
+                                      IOReadHandler *fd_read,
+                                      IOCanRWHandler *fd_can_read,
+                                      VLANClientCleanupHandler *cleanup,
+                                      void *opaque)
+{
+    VLANClientState *vc, **pvc;
+    vc = new_vlan_client(vlan, model, name, fd_read, fd_can_read, cleanup,
+                         opaque);
 
     vc->next = NULL;
     pvc = &vlan->first_client;
@@ -357,6 +373,23 @@ VLANClientState *qemu_new_vlan_client(VLANState *vlan,
     return vc;
 }
 
+VLANClientState *qemu_new_vlan_head_client(VLANState *vlan,
+                                           const char *model,
+                                           const char *name,
+                                           IOReadHandler *fd_read,
+                                           IOCanRWHandler *fd_can_read,
+                                           VLANClientCleanupHandler *cleanup,
+                                           void *opaque)
+{
+    VLANClientState *vc;
+    vc = new_vlan_client(vlan, model, name, fd_read, fd_can_read, cleanup,
+                         opaque);
+
+    vc->next = vlan->first_client;
+    vlan->first_client = vc;
+    return vc;
+}
+
 void qemu_del_vlan_client(VLANClientState *vc)
 {
     VLANClientState **pvc = &vc->vlan->first_client;
@@ -1571,6 +1604,106 @@ static int net_socket_mcast_init(VLANState *vlan,
 
 }
 
+typedef struct DumpState {
+    VLANClientState *pcap_vc;
+    int fd;
+    int pcap_caplen;
+} DumpState;
+
+#define PCAP_MAGIC 0xa1b2c3d4
+
+struct pcap_file_hdr {
+    uint32_t magic;
+    uint16_t version_major;
+    uint16_t version_minor;
+    int32_t thiszone;
+    uint32_t sigfigs;
+    uint32_t snaplen;
+    uint32_t linktype;
+};
+
+struct pcap_sf_pkthdr {
+    struct {
+        int32_t tv_sec;
+        int32_t tv_usec;
+    } ts;
+    uint32_t caplen;
+    uint32_t len;
+};
+
+static void dump_receive(void *opaque, const uint8_t *buf, int size)
+{
+    DumpState *s = opaque;
+    struct pcap_sf_pkthdr hdr;
+    int64_t ts;
+    int caplen;
+
+    /* Early return in case of previous error. */
+    if (s->fd < 0) {
+        return;
+    }
+
+    ts = muldiv64 (qemu_get_clock(vm_clock),1000000, ticks_per_sec);
+    caplen = size > s->pcap_caplen ? s->pcap_caplen : size;
+
+    hdr.ts.tv_sec = ts / 1000000000LL;
+    hdr.ts.tv_usec = ts % 1000000;
+    hdr.caplen = caplen;
+    hdr.len = size;
+    if (write(s->fd, &hdr, sizeof(hdr)) != sizeof(hdr) ||
+        write(s->fd, buf, caplen) != caplen) {
+        qemu_log("-net dump write error - stop dump\n");
+        close(s->fd);
+        s->fd = -1;
+    }
+}
+
+static void net_dump_cleanup(void *opaque)
+{
+    DumpState *s = opaque;
+
+    close(s->fd);
+    qemu_free(s);
+}
+
+static int net_dump_init(VLANState *vlan, const char *device,
+                         const char *name, const char *filename, int len)
+{
+    struct pcap_file_hdr hdr;
+    DumpState *s;
+
+    s = qemu_malloc(sizeof(DumpState));
+
+    s->fd = open(filename, O_CREAT | O_WRONLY, 0644);
+    if (s->fd < 0) {
+        fprintf(stderr, "-net dump: can't open %s\n", filename);
+        return -1;
+    }
+
+    s->pcap_caplen = len;
+
+    hdr.magic = PCAP_MAGIC;
+    hdr.version_major = 2;
+    hdr.version_minor = 4;
+    hdr.thiszone = 0;
+    hdr.sigfigs = 0;
+    hdr.snaplen = s->pcap_caplen;
+    hdr.linktype = 1;
+
+    if (write(s->fd, &hdr, sizeof(hdr)) < sizeof(hdr)) {
+        perror("-net dump write error");
+        close(s->fd);
+        qemu_free(s);
+        return -1;
+    }
+
+    s->pcap_vc = qemu_new_vlan_head_client(vlan, device, name, dump_receive,
+                                           NULL, net_dump_cleanup, s);
+    snprintf(s->pcap_vc->info_str, sizeof(s->pcap_vc->info_str),
+             "dump to %s (len=%d)", filename, len);
+    return 0;
+}
+
 /* find or alloc a new VLAN */
 VLANState *qemu_find_vlan(int id)
 {
@@ -1811,7 +1944,17 @@ int net_client_init(const char *device, const char *p)
 	ret = net_vde_init(vlan, device, name, vde_sock, vde_port, vde_group, vde_mode);
     } else
 #endif
-    {
+    if (!strcmp(device, "dump")) {
+        int len = 65536;
+
+        if (get_param_value(buf, sizeof(buf), "len", p) > 0) {
+            len = strtol(buf, NULL, 0);
+        }
+        if (!get_param_value(buf, sizeof(buf), "file", p)) {
+            snprintf(buf, sizeof(buf), "qemu-vlan%d.pcap", vlan_id);
+        }
+        ret = net_dump_init(vlan, device, name, buf, len);
+    } else {
         fprintf(stderr, "Unknown network device: %s\n", device);
         if (name)
             free(name);
@@ -1836,7 +1979,7 @@ void net_client_uninit(NICInfo *nd)
 static int net_host_check_device(const char *device)
 {
     int i;
-    const char *valid_param_list[] = { "tap", "socket"
+    const char *valid_param_list[] = { "tap", "socket", "dump"
 #ifdef CONFIG_SLIRP
                                        ,"user"
 #endif
diff --git a/net.h b/net.h
index 571305b..6560d0d 100644
--- a/net.h
+++ b/net.h
@@ -45,6 +45,13 @@ VLANClientState *qemu_new_vlan_client(VLANState *vlan,
                                       IOCanRWHandler *fd_can_read,
                                       VLANClientCleanupHandler *cleanup,
                                       void *opaque);
+VLANClientState *qemu_new_vlan_head_client(VLANState *vlan,
+                                           const char *model,
+                                           const char *name,
+                                           IOReadHandler *fd_read,
+                                           IOCanRWHandler *fd_can_read,
+                                           VLANClientCleanupHandler *cleanup,
+                                           void *opaque);
 void qemu_del_vlan_client(VLANClientState *vc);
 VLANClientState *qemu_find_vlan_client(VLANState *vlan, void *opaque);
 int qemu_can_send_packet(VLANClientState *vc);
diff --git a/qemu-options.hx b/qemu-options.hx
index f551775..ab19bd3 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -724,6 +724,8 @@ DEF("net", HAS_ARG, QEMU_OPTION_net, \
     "                Use group 'groupname' and mode 'octalmode' to change default\n"
     "                ownership and permissions for communication port.\n"
 #endif
+    "-net dump[,vlan=n][,file=f][,len=n]\n"
+    "                dump traffic on vlan 'n' to file 'f' (max n bytes per packet)\n"
     "-net none       use it alone to have zero network devices; if no -net option\n"
     "                is provided, the default is '-net nic -net user'\n")
 STEXI
@@ -844,6 +846,11 @@ vde_switch -F -sock /tmp/myswitch
 qemu linux.img -net nic -net vde,sock=/tmp/myswitch
 @end example
 
+@item -net dump[,vlan=@var{n}][,file=@var{file}][,len=@var{len}]
+Dump network traffic on VLAN @var{n} to file @var{file} (@file{qemu-vlan0.pcap} by default).
+At most @var{len} bytes (64k by default) per packet are stored. The file format is
+libpcap, so it can be analyzed with tools such as tcpdump or Wireshark.
+
 @item -net none
 Indicate that no network devices should be configured. It is used to
 override the default configuration (@option{-net nic -net user}) which

Re: [Qemu-devel] [PATCH 6/7] net: Add support for capturing VLANs

[Mark McLoughlin]

On Tue, 2009-04-14 at 19:29 +0200, Jan Kiszka wrote:
> This patch is derived from Tristan Gingold's patch. It adds a new VLAN
> client type that writes all traffic on the VLAN it is attached to into a
> pcap file. Such a file can then be analyzed offline with Wireshark or
> tcpdump.

Personally, I'd use a tap device and 'tcpdump -w' to do this.

Any particular reason to add support to qemu itself other than
convenience?

> Besides rebasing and some minor cleanups, the major differences to the
> original version are:
>  - support for enabling/disabling via the monitor (host_net_add/remove)
>  - always register dump client at the head of a VLAN queue
>    (instead of special handling for slirp)

Could you explain why you need this? 

I'd prefer if we didn't have to add qemu_new_vlan_head_client()

Cheers,
Mark.

[Qemu-devel] Re: [PATCH 6/7] net: Add support for capturing VLANs

[Jan Kiszka]

Mark McLoughlin wrote:
> On Tue, 2009-04-14 at 19:29 +0200, Jan Kiszka wrote:
>> This patch is derived from Tristan Gingold's patch. It adds a new VLAN
>> client type that writes all traffic on the VLAN it is attached to into a
>> pcap file. Such a file can then be analyzed offline with Wireshark or
>> tcpdump.
> 
> Personally, I'd use a tap device and 'tcpdump -w' to do this.
> 
> Any particular reason to add support to qemu itself other than
> convenience?

You have to have the right privileges (for me the key argument) + you
have to add the tap device in the right order, see below. If we were
talking about thousands of LoC, I would say, let's stick with these
limitations. But the capturing features is really small and
self-contained (you may also consider it as a feature extension of
slirp, which you could basically also replace with a tap device +
iptables rules).

> 
>> Besides rebasing and some minor cleanups, the major differences to the
>> original version are:
>>  - support for enabling/disabling via the monitor (host_net_add/remove)
>>  - always register dump client at the head of a VLAN queue
>>    (instead of special handling for slirp)
> 
> Could you explain why you need this? 
> 
> I'd prefer if we didn't have to add qemu_new_vlan_head_client()

Packet ordering: If you are sniffing from behind a vlan client in the
queue, you may see its immediate reply to a certain packet before the
triggering packet. Tristan solved this by pushing slirp (the only source
for reordering so far) at the end of the queue, but I think it's rather
the sniffer which has special requirements, so I pushed that one to the top.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 6/7] net: Add support for capturing VLANs

[Mark McLoughlin]

On Wed, 2009-04-15 at 16:09 +0200, Jan Kiszka wrote:
> >> Besides rebasing and some minor cleanups, the major differences to the
> >> original version are:
> >>  - support for enabling/disabling via the monitor (host_net_add/remove)
> >>  - always register dump client at the head of a VLAN queue
> >>    (instead of special handling for slirp)
> > 
> > Could you explain why you need this? 
> > 
> > I'd prefer if we didn't have to add qemu_new_vlan_head_client()
> 
> Packet ordering: If you are sniffing from behind a vlan client in the
> queue, you may see its immediate reply to a certain packet before the
> triggering packet. Tristan solved this by pushing slirp (the only source
> for reordering so far) at the end of the queue, but I think it's rather
> the sniffer which has special requirements, so I pushed that one to the top.

Uggh, nasty ... slirp strikes again :-)

If we need to make the sniffer a special case, I'd suggest really making
it a special case by doing e.g.

static void dump_packet(Dumper *d, const uint8_t *buf, int size)
{
    /* write packet to pcap file */
}

void qemu_send_packet(...)
{
    if (vlan->dumper)
        dump_packet(d, buf, size);

    /* dispatch packets to vlan clients */
}

I think making it explicit is better than munging the vlan abstraction
any further.

Alternatively, we could fix slirp to push packets to a queue and flush
that queue in a bottom half.

Cheers,
Mark.

[Qemu-devel] Re: [PATCH 6/7] net: Add support for capturing VLANs

[Jan Kiszka]

Mark McLoughlin wrote:
> On Wed, 2009-04-15 at 16:09 +0200, Jan Kiszka wrote:
>>>> Besides rebasing and some minor cleanups, the major differences to the
>>>> original version are:
>>>>  - support for enabling/disabling via the monitor (host_net_add/remove)
>>>>  - always register dump client at the head of a VLAN queue
>>>>    (instead of special handling for slirp)
>>> Could you explain why you need this? 
>>>
>>> I'd prefer if we didn't have to add qemu_new_vlan_head_client()
>> Packet ordering: If you are sniffing from behind a vlan client in the
>> queue, you may see its immediate reply to a certain packet before the
>> triggering packet. Tristan solved this by pushing slirp (the only source
>> for reordering so far) at the end of the queue, but I think it's rather
>> the sniffer which has special requirements, so I pushed that one to the top.
> 
> Uggh, nasty ... slirp strikes again :-)
> 
> If we need to make the sniffer a special case, I'd suggest really making
> it a special case by doing e.g.
> 
> static void dump_packet(Dumper *d, const uint8_t *buf, int size)
> {
>     /* write packet to pcap file */
> }
> 
> void qemu_send_packet(...)
> {
>     if (vlan->dumper)
>         dump_packet(d, buf, size);
> 
>     /* dispatch packets to vlan clients */
> }
> 
> I think making it explicit is better than munging the vlan abstraction
> any further.
> 
> Alternatively, we could fix slirp to push packets to a queue and flush
> that queue in a bottom half.

That's actually a good idea - in case something else once comes around
that gets confused by packet ordering. I'll check if we can do this more
or less transparently in the network layer (queue tx requests while
delivering). Shouldn't be hard (at least not as hard as changing slirp ;) ).

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 5/7] monitor: Improve host_net_add

[Jan Kiszka]

Fix the documentation of the host_net_add monitor command and allow the
user to pass no options at all. Moreover, inform the user on the
monitor terminal if a request failed.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 monitor.c |    4 ++--
 net.c     |    4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index e764b5d..d9f84a7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1730,8 +1730,8 @@ static const mon_cmd_t mon_cmds[] = {
                                         "add drive to PCI storage controller" },
     { "pci_add", "sss", pci_device_hot_add, "pci_addr=auto|[[<domain>:]<bus>:]<slot> nic|storage [[vlan=n][,macaddr=addr][,model=type]] [file=file][,if=type][,bus=nr]...", "hot-add PCI device" },
     { "pci_del", "s", pci_device_hot_remove, "pci_addr=[[<domain>:]<bus>:]<slot>", "hot remove PCI device" },
-    { "host_net_add", "ss", net_host_device_add,
-      "[tap,user,socket,vde] options", "add host VLAN client" },
+    { "host_net_add", "ss?", net_host_device_add,
+      "tap|user|socket|vde [options]", "add host VLAN client" },
     { "host_net_remove", "is", net_host_device_remove,
       "vlan_id name", "remove host VLAN client" },
 #endif
diff --git a/net.c b/net.c
index 0486f7c..e812eb8 100644
--- a/net.c
+++ b/net.c
@@ -1859,7 +1859,9 @@ void net_host_device_add(Monitor *mon, const char *device, const char *opts)
         monitor_printf(mon, "invalid host network device %s\n", device);
         return;
     }
-    net_client_init(device, opts);
+    if (net_client_init(device, opts ? : "") < 0) {
+        monitor_printf(mon, "adding host network device %s failed\n", device);
+    }
 }
 
 void net_host_device_remove(Monitor *mon, int vlan_id, const char *device)

Re: [Qemu-devel] [PATCH 5/7] monitor: Improve host_net_add

[Mark McLoughlin]

On Tue, 2009-04-14 at 19:29 +0200, Jan Kiszka wrote:
> Fix the documentation of the host_net_add monitor command and allow the
> user to pass no options at all. Moreover, inform the user on the
> monitor terminal if a request failed.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  monitor.c |    4 ++--
>  net.c     |    4 +++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index e764b5d..d9f84a7 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1730,8 +1730,8 @@ static const mon_cmd_t mon_cmds[] = {
>                                          "add drive to PCI storage controller" },
>      { "pci_add", "sss", pci_device_hot_add, "pci_addr=auto|[[<domain>:]<bus>:]<slot> nic|storage [[vlan=n][,macaddr=addr][,model=type]] [file=file][,if=type][,bus=nr]...", "hot-add PCI device" },
>      { "pci_del", "s", pci_device_hot_remove, "pci_addr=[[<domain>:]<bus>:]<slot>", "hot remove PCI device" },
> -    { "host_net_add", "ss", net_host_device_add,
> -      "[tap,user,socket,vde] options", "add host VLAN client" },
> +    { "host_net_add", "ss?", net_host_device_add,
> +      "tap|user|socket|vde [options]", "add host VLAN client" },
>      { "host_net_remove", "is", net_host_device_remove,
>        "vlan_id name", "remove host VLAN client" },
>  #endif
> diff --git a/net.c b/net.c
> index 0486f7c..e812eb8 100644
> --- a/net.c
> +++ b/net.c
> @@ -1859,7 +1859,9 @@ void net_host_device_add(Monitor *mon, const char *device, const char *opts)
>          monitor_printf(mon, "invalid host network device %s\n", device);
>          return;
>      }
> -    net_client_init(device, opts);
> +    if (net_client_init(device, opts ? : "") < 0) {
> +        monitor_printf(mon, "adding host network device %s failed\n", device);
> +    }
>  }

Looks good to me.

Acked-by: Mark McLoughlin <markmc@redhat.com>

Cheers,
Mark.

[Qemu-devel] [PATCH 7/7] slirp: Handle DHCP requests for specific IP

[Jan Kiszka]

This adds proper handling of the ciaddr field as well as the "Requested
IP Address" option to slirp's DHCP server. If the client requests an
invalid or used IP, a NAK reply is sent, if it requests a specific but
valid IP, this is now respected.

NAK'ing invalid IPs is specifically useful when changing the slirp IP
range via '-net user,ip=...' while the client saved its previously used
address and tries to reacquire it. Now this will be NAK'ed and the
client will start a new discovery round.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 slirp/bootp.c |  127 ++++++++++++++++++++++++++++++++++++++++++---------------
 slirp/bootp.h |    2 +
 2 files changed, 95 insertions(+), 34 deletions(-)

diff --git a/slirp/bootp.c b/slirp/bootp.c
index ca177f4..8a97660 100644
--- a/slirp/bootp.c
+++ b/slirp/bootp.c
@@ -66,6 +66,24 @@ static BOOTPClient *get_new_addr(struct in_addr *paddr)
     return bc;
 }
 
+static BOOTPClient *request_addr(const struct in_addr *paddr,
+                                 const uint8_t *macaddr)
+{
+    uint32_t req_addr = ntohl(paddr->s_addr);
+    uint32_t spec_addr = ntohl(special_addr.s_addr);
+    BOOTPClient *bc;
+
+    if (req_addr >= (spec_addr | START_ADDR) &&
+        req_addr < (spec_addr | (NB_ADDR + START_ADDR))) {
+        bc = &bootp_clients[(req_addr & 0xff) - START_ADDR];
+        if (!bc->allocated || !memcmp(macaddr, bc->macaddr, 6)) {
+            bc->allocated = 1;
+            return bc;
+        }
+    }
+    return NULL;
+}
+
 static BOOTPClient *find_addr(struct in_addr *paddr, const uint8_t *macaddr)
 {
     BOOTPClient *bc;
@@ -83,18 +101,17 @@ static BOOTPClient *find_addr(struct in_addr *paddr, const uint8_t *macaddr)
     return bc;
 }
 
-static void dhcp_decode(const uint8_t *buf, int size,
-                        int *pmsg_type)
+static void dhcp_decode(const struct bootp_t *bp, int *pmsg_type,
+                        const struct in_addr **preq_addr)
 {
     const uint8_t *p, *p_end;
     int len, tag;
 
     *pmsg_type = 0;
+    *preq_addr = NULL;
 
-    p = buf;
-    p_end = buf + size;
-    if (size < 5)
-        return;
+    p = bp->bp_vend;
+    p_end = p + DHCP_OPT_LEN;
     if (memcmp(p, rfc1533_cookie, 4) != 0)
         return;
     p += 4;
@@ -109,34 +126,46 @@ static void dhcp_decode(const uint8_t *buf, int size,
             if (p >= p_end)
                 break;
             len = *p++;
-            dprintf("dhcp: tag=0x%02x len=%d\n", tag, len);
+            dprintf("dhcp: tag=%d len=%d\n", tag, len);
 
             switch(tag) {
             case RFC2132_MSG_TYPE:
                 if (len >= 1)
                     *pmsg_type = p[0];
                 break;
+            case RFC2132_REQ_ADDR:
+                if (len >= 4)
+                    *preq_addr = (struct in_addr *)p;
+                break;
             default:
                 break;
             }
             p += len;
         }
     }
+    if (*pmsg_type == DHCPREQUEST && !*preq_addr && bp->bp_ciaddr.s_addr) {
+        *preq_addr = &bp->bp_ciaddr;
+    }
 }
 
-static void bootp_reply(struct bootp_t *bp)
+static void bootp_reply(const struct bootp_t *bp)
 {
-    BOOTPClient *bc;
+    BOOTPClient *bc = NULL;
     struct mbuf *m;
     struct bootp_t *rbp;
     struct sockaddr_in saddr, daddr;
     struct in_addr dns_addr;
+    const struct in_addr *preq_addr;
     int dhcp_msg_type, val;
     uint8_t *q;
 
     /* extract exact DHCP msg type */
-    dhcp_decode(bp->bp_vend, DHCP_OPT_LEN, &dhcp_msg_type);
-    dprintf("bootp packet op=%d msgtype=%d\n", bp->bp_op, dhcp_msg_type);
+    dhcp_decode(bp, &dhcp_msg_type, &preq_addr);
+    dprintf("bootp packet op=%d msgtype=%d", bp->bp_op, dhcp_msg_type);
+    if (preq_addr)
+        dprintf(" req_addr=%08x\n", ntohl(preq_addr->s_addr));
+    else
+        dprintf("\n");
 
     if (dhcp_msg_type == 0)
         dhcp_msg_type = DHCPREQUEST; /* Force reply for old BOOTP clients */
@@ -155,13 +184,29 @@ static void bootp_reply(struct bootp_t *bp)
     memset(rbp, 0, sizeof(struct bootp_t));
 
     if (dhcp_msg_type == DHCPDISCOVER) {
-    new_addr:
-        bc = get_new_addr(&daddr.sin_addr);
+        if (preq_addr) {
+            bc = request_addr(preq_addr, client_ethaddr);
+            if (bc) {
+                daddr.sin_addr = *preq_addr;
+            }
+        }
         if (!bc) {
-            dprintf("no address left\n");
-            return;
+         new_addr:
+            bc = get_new_addr(&daddr.sin_addr);
+            if (!bc) {
+                dprintf("no address left\n");
+                return;
+            }
         }
         memcpy(bc->macaddr, client_ethaddr, 6);
+    } else if (preq_addr) {
+        bc = request_addr(preq_addr, client_ethaddr);
+        if (bc) {
+            daddr.sin_addr = *preq_addr;
+            memcpy(bc->macaddr, client_ethaddr, 6);
+        } else {
+            daddr.sin_addr.s_addr = 0;
+        }
     } else {
         bc = find_addr(&daddr.sin_addr, bp->bp_hwaddr);
         if (!bc) {
@@ -171,12 +216,6 @@ static void bootp_reply(struct bootp_t *bp)
         }
     }
 
-    if (bootp_filename)
-        snprintf((char *)rbp->bp_file, sizeof(rbp->bp_file), "%s",
-                 bootp_filename);
-
-    dprintf("offered addr=%08x\n", ntohl(daddr.sin_addr.s_addr));
-
     saddr.sin_addr.s_addr = htonl(ntohl(special_addr.s_addr) | CTL_ALIAS);
     saddr.sin_port = htons(BOOTP_SERVER);
 
@@ -191,24 +230,29 @@ static void bootp_reply(struct bootp_t *bp)
     rbp->bp_yiaddr = daddr.sin_addr; /* Client IP address */
     rbp->bp_siaddr = saddr.sin_addr; /* Server IP address */
 
-    daddr.sin_addr.s_addr = 0xffffffffu;
-
     q = rbp->bp_vend;
     memcpy(q, rfc1533_cookie, 4);
     q += 4;
 
-    if (dhcp_msg_type == DHCPDISCOVER) {
-        *q++ = RFC2132_MSG_TYPE;
-        *q++ = 1;
-        *q++ = DHCPOFFER;
-    } else if (dhcp_msg_type == DHCPREQUEST) {
-        *q++ = RFC2132_MSG_TYPE;
-        *q++ = 1;
-        *q++ = DHCPACK;
-    }
+    if (bc) {
+        dprintf("%s addr=%08x\n",
+                (dhcp_msg_type == DHCPDISCOVER) ? "offered" : "ack'ed",
+                ntohl(daddr.sin_addr.s_addr));
+
+        if (dhcp_msg_type == DHCPDISCOVER) {
+            *q++ = RFC2132_MSG_TYPE;
+            *q++ = 1;
+            *q++ = DHCPOFFER;
+        } else /* DHCPREQUEST */ {
+            *q++ = RFC2132_MSG_TYPE;
+            *q++ = 1;
+            *q++ = DHCPACK;
+        }
+
+        if (bootp_filename)
+            snprintf((char *)rbp->bp_file, sizeof(rbp->bp_file), "%s",
+                     bootp_filename);
 
-    if (dhcp_msg_type == DHCPDISCOVER ||
-        dhcp_msg_type == DHCPREQUEST) {
         *q++ = RFC2132_SRV_ID;
         *q++ = 4;
         memcpy(q, &saddr.sin_addr, 4);
@@ -247,9 +291,24 @@ static void bootp_reply(struct bootp_t *bp)
             memcpy(q, slirp_hostname, val);
             q += val;
         }
+    } else {
+        static const char nak_msg[] = "requested address not available";
+
+        dprintf("nak'ed addr=%08x\n", ntohl(preq_addr->s_addr));
+
+        *q++ = RFC2132_MSG_TYPE;
+        *q++ = 1;
+        *q++ = DHCPNAK;
+
+        *q++ = RFC2132_MESSAGE;
+        *q++ = sizeof(nak_msg) - 1;
+        memcpy(q, nak_msg, sizeof(nak_msg) - 1);
+        q += sizeof(nak_msg) - 1;
     }
     *q++ = RFC1533_END;
 
+    daddr.sin_addr.s_addr = 0xffffffffu;
+
     m->m_len = sizeof(struct bootp_t) -
         sizeof(struct ip) - sizeof(struct udphdr);
     udp_output2(NULL, m, &saddr, &daddr, IPTOS_LOWDELAY);
diff --git a/slirp/bootp.h b/slirp/bootp.h
index e48f53f..3d80515 100644
--- a/slirp/bootp.h
+++ b/slirp/bootp.h
@@ -63,6 +63,7 @@
 #define RFC2132_MSG_TYPE	53
 #define RFC2132_SRV_ID		54
 #define RFC2132_PARAM_LIST	55
+#define RFC2132_MESSAGE		56
 #define RFC2132_MAX_SIZE	57
 #define RFC2132_RENEWAL_TIME    58
 #define RFC2132_REBIND_TIME     59
@@ -71,6 +72,7 @@
 #define DHCPOFFER		2
 #define DHCPREQUEST		3
 #define DHCPACK			5
+#define DHCPNAK			6
 
 #define RFC1533_VENDOR_MAJOR	0
 #define RFC1533_VENDOR_MINOR	0