[Qemu-devel] [PATCH 0/9] Misc networking fixes

[Qemu-devel] [PATCH 0/9] Misc networking fixes

[Mark McLoughlin]

Hi Anthony,
        Here are a bunch of networking fixes.

        The only really non-trivial one is final one, which adds
VLANClientState->cleanup(). This is similar to Jan's patch, but
more comprehensive, I think.

        Note, these patches aren't as well tested as I'd like. I'm
finding I have various issues with image corruption and PCI hotplug not
working at the moment. However, I have compile tested on all targets
and tested basic networking functionality, including e.g. host_net_remove.

Cheers,
Mark.

[Qemu-devel] [PATCH 1/9] Remove stray GSO code from virtio_net

[Mark McLoughlin]

Obviously merged from kvm-userspace accidentally.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 hw/virtio-net.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 88ec1ac..5e7db0d 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -338,11 +338,6 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
     if (n->promisc)
         return 1;
 
-#ifdef TAP_VNET_HDR
-    if (tap_has_vnet_hdr(n->vc->vlan->first_client))
-        ptr += sizeof(struct virtio_net_hdr);
-#endif
-
     if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
         int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
         if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
-- 
1.6.0.6

[Qemu-devel] [PATCH 2/9] struct iovec is now universally available

[Mark McLoughlin]

struct iovec is now defined in qemu-common.h if needed, so we don't need
the tap code to handle !defined(HAVE_IOVEC).

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/net.c b/net.c
index 5365891..5e6895c 100644
--- a/net.c
+++ b/net.c
@@ -702,7 +702,6 @@ typedef struct TAPState {
     char down_script_arg[128];
 } TAPState;
 
-#ifdef HAVE_IOVEC
 static ssize_t tap_receive_iov(void *opaque, const struct iovec *iov,
                                int iovcnt)
 {
@@ -715,7 +714,6 @@ static ssize_t tap_receive_iov(void *opaque, const struct iovec *iov,
 
     return len;
 }
-#endif
 
 static void tap_receive(void *opaque, const uint8_t *buf, int size)
 {
@@ -762,9 +760,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);
-#ifdef HAVE_IOVEC
     s->vc->fd_readv = tap_receive_iov;
-#endif
     qemu_set_fd_handler(s->fd, tap_send, NULL, s);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str), "fd=%d", fd);
     return s;
-- 
1.6.0.6

[Qemu-devel] [PATCH 3/9] Fix error handling in net_client_init()

[Mark McLoughlin]

We weren't freeing the name string everywhere.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |   26 ++++++++++++++++----------
 1 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/net.c b/net.c
index 5e6895c..2383b10 100644
--- a/net.c
+++ b/net.c
@@ -1610,7 +1610,8 @@ int net_client_init(const char *device, const char *p)
 
         if (idx == -1 || nb_nics >= MAX_NICS) {
             fprintf(stderr, "Too Many NICs\n");
-            return -1;
+            ret = -1;
+            goto out;
         }
         nd = &nd_table[idx];
         macaddr = nd->macaddr;
@@ -1624,7 +1625,8 @@ int net_client_init(const char *device, const char *p)
         if (get_param_value(buf, sizeof(buf), "macaddr", p)) {
             if (parse_macaddr(macaddr, buf) < 0) {
                 fprintf(stderr, "invalid syntax for ethernet address\n");
-                return -1;
+                ret = -1;
+                goto out;
             }
         }
         if (get_param_value(buf, sizeof(buf), "model", p)) {
@@ -1664,8 +1666,9 @@ int net_client_init(const char *device, const char *p)
         port = strtol(p, &devname, 10);
         devname++;
         if (port < 1 || port > 65535) {
-            fprintf(stderr, "vmchannel wrong port number\n"); 
-            return -1;
+            fprintf(stderr, "vmchannel wrong port number\n");
+            ret = -1;
+            goto out;
         }
         vmc = malloc(sizeof(struct VMChannel));
         snprintf(name, 20, "vmchannel%ld", port);
@@ -1673,7 +1676,8 @@ int net_client_init(const char *device, const char *p)
         if (!vmc->hd) {
             fprintf(stderr, "qemu: could not open vmchannel device"
                     "'%s'\n", devname);
-            return -1;
+            ret = -1;
+            goto out;
         }
         vmc->port = port;
         slirp_add_exec(3, vmc->hd, 4, port);
@@ -1687,7 +1691,8 @@ int net_client_init(const char *device, const char *p)
         char ifname[64];
         if (get_param_value(ifname, sizeof(ifname), "ifname", p) <= 0) {
             fprintf(stderr, "tap: no interface name\n");
-            return -1;
+            ret = -1;
+            goto out;
         }
         vlan->nb_host_devs++;
         ret = tap_win32_init(vlan, device, name, ifname);
@@ -1734,7 +1739,8 @@ int net_client_init(const char *device, const char *p)
             ret = net_socket_mcast_init(vlan, device, name, buf);
         } else {
             fprintf(stderr, "Unknown socket options: %s\n", p);
-            return -1;
+            ret = -1;
+            goto out;
         }
         vlan->nb_host_devs++;
     } else
@@ -1764,13 +1770,13 @@ int net_client_init(const char *device, const char *p)
 #endif
     {
         fprintf(stderr, "Unknown network device: %s\n", device);
-        if (name)
-            free(name);
-        return -1;
+        ret = -1;
+        goto out;
     }
     if (ret < 0) {
         fprintf(stderr, "Could not initialize device '%s'\n", device);
     }
+out:
     if (name)
         free(name);
     return ret;
-- 
1.6.0.6

[Qemu-devel] [PATCH 4/9] Don't fail PCI hotplug if no NIC model is supplied

[Mark McLoughlin]

It's perfectly fine to not supply a NIC model when adding
a new NIC - we supply the default model to pci_nic_init()
and it uses that if one wasn't explicitly supplied.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 hw/pci-hotplug.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index d968a14..603d74d 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -37,10 +37,10 @@ static PCIDevice *qemu_pci_hot_add_nic(PCIBus *pci_bus, const char *opts)
 {
     int ret;
 
-    ret = net_client_init ("nic", opts);
-    if (ret < 0 || !nd_table[ret].model)
+    ret = net_client_init("nic", opts);
+    if (ret < 0)
         return NULL;
-    return pci_nic_init (pci_bus, &nd_table[ret], -1, "rtl8139");
+    return pci_nic_init(pci_bus, &nd_table[ret], -1, "rtl8139");
 }
 
 void drive_hot_add(Monitor *mon, const char *pci_addr, const char *opts)
-- 
1.6.0.6

[Qemu-devel] [PATCH 5/9] Remove some useless malloc() checking

[Mark McLoughlin]

Now that we abort() on malloc, neither qemu_find_vlan() nor
net_tap_fd_init() can fail.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |   16 +++-------------
 1 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/net.c b/net.c
index 2383b10..34ec4c8 100644
--- a/net.c
+++ b/net.c
@@ -1015,8 +1015,6 @@ static int net_tap_init(VLANState *vlan, const char *model,
 	    return -1;
     }
     s = net_tap_fd_init(vlan, model, name, fd);
-    if (!s)
-        return -1;
     snprintf(s->vc->info_str, sizeof(s->vc->info_str),
              "ifname=%s,script=%s,downscript=%s",
              ifname, setup_script, down_script);
@@ -1596,10 +1594,7 @@ int net_client_init(const char *device, const char *p)
         vlan_id = strtol(buf, NULL, 0);
     }
     vlan = qemu_find_vlan(vlan_id);
-    if (!vlan) {
-        fprintf(stderr, "Could not create vlan %d\n", vlan_id);
-        return -1;
-    }
+
     if (get_param_value(buf, sizeof(buf), "name", p)) {
         name = strdup(buf);
     }
@@ -1707,9 +1702,8 @@ int net_client_init(const char *device, const char *p)
         if (get_param_value(buf, sizeof(buf), "fd", p) > 0) {
             fd = strtol(buf, NULL, 0);
             fcntl(fd, F_SETFL, O_NONBLOCK);
-            ret = -1;
-            if (net_tap_fd_init(vlan, device, name, fd))
-                ret = 0;
+            net_tap_fd_init(vlan, device, name, fd);
+            ret = 0;
         } else {
             if (get_param_value(ifname, sizeof(ifname), "ifname", p) <= 0) {
                 ifname[0] = '\0';
@@ -1825,10 +1819,6 @@ void net_host_device_remove(Monitor *mon, int vlan_id, const char *device)
     VLANClientState *vc;
 
     vlan = qemu_find_vlan(vlan_id);
-    if (!vlan) {
-        monitor_printf(mon, "can't find vlan %d\n", vlan_id);
-        return;
-    }
 
    for(vc = vlan->first_client; vc != NULL; vc = vc->next)
         if (!strcmp(vc->name, device))
-- 
1.6.0.6

[Qemu-devel] [PATCH 6/9] Remove NICInfo from e1000 and mipsnet state

[Mark McLoughlin]

NICInfo isn't used after initialization, so remove it from the driver
state structures.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 hw/e1000.c   |    4 +---
 hw/mipsnet.c |    4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 1644201..2d16774 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -75,7 +75,6 @@ enum {
 typedef struct E1000State_st {
     PCIDevice dev;
     VLANClientState *vc;
-    NICInfo *nd;
     int mmio_index;
 
     uint32_t mac_reg[0x8000];
@@ -1078,7 +1077,6 @@ pci_e1000_init(PCIBus *bus, NICInfo *nd, int devfn)
     pci_register_io_region((PCIDevice *)d, 1, IOPORT_SIZE,
                            PCI_ADDRESS_SPACE_IO, ioport_map);
 
-    d->nd = nd;
     memmove(d->eeprom_data, e1000_eeprom_template,
         sizeof e1000_eeprom_template);
     for (i = 0; i < 3; i++)
@@ -1099,7 +1097,7 @@ pci_e1000_init(PCIBus *bus, NICInfo *nd, int devfn)
                                  e1000_receive, e1000_can_receive, d);
     d->vc->link_status_changed = e1000_set_link_status;
 
-    qemu_format_nic_info_str(d->vc, d->nd->macaddr);
+    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;
diff --git a/hw/mipsnet.c b/hw/mipsnet.c
index 29bd9b8..415b04e 100644
--- a/hw/mipsnet.c
+++ b/hw/mipsnet.c
@@ -35,7 +35,6 @@ typedef struct MIPSnetState {
     uint8_t tx_buffer[MAX_ETH_FRAME_SIZE];
     qemu_irq irq;
     VLANClientState *vc;
-    NICInfo *nd;
 } MIPSnetState;
 
 static void mipsnet_reset(MIPSnetState *s)
@@ -248,7 +247,6 @@ void mipsnet_init (int base, qemu_irq irq, NICInfo *nd)
     register_ioport_read(base, 36, 4, mipsnet_ioport_read, s);
 
     s->irq = irq;
-    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);
@@ -256,7 +254,7 @@ void mipsnet_init (int base, qemu_irq irq, NICInfo *nd)
         s->vc = NULL;
     }
 
-    qemu_format_nic_info_str(s->vc, s->nd->macaddr);
+    qemu_format_nic_info_str(s->vc, nd->macaddr);
 
     mipsnet_reset(s);
     register_savevm("mipsnet", 0, 0, mipsnet_save, mipsnet_load, s);
-- 
1.6.0.6

[Qemu-devel] [PATCH 7/9] Add unregister_savevm()

[Mark McLoughlin]

Currently there's no way to unregister a savevm callback, so
e.g. if a NIC is hot-unplugged and a savevm is issued, we'll
segfault.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 hw/hw.h  |    2 ++
 savevm.c |   16 ++++++++++++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index e9628d4..d0cf598 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -239,6 +239,8 @@ int register_savevm_live(const char *idstr,
                          LoadStateHandler *load_state,
                          void *opaque);
 
+void unregister_savevm(const char *idstr, void *opaque);
+
 typedef void QEMUResetHandler(void *opaque);
 
 void qemu_register_reset(QEMUResetHandler *func, void *opaque);
diff --git a/savevm.c b/savevm.c
index 70500dd..c15db9a 100644
--- a/savevm.c
+++ b/savevm.c
@@ -647,6 +647,22 @@ int register_savevm(const char *idstr,
                                 NULL, save_state, load_state, opaque);
 }
 
+void unregister_savevm(const char *idstr, void *opaque)
+{
+    SaveStateEntry **pse;
+
+    pse = &first_se;
+    while (*pse != NULL) {
+        if (strcmp((*pse)->idstr, idstr) == 0 && (*pse)->opaque == opaque) {
+            SaveStateEntry *next = (*pse)->next;
+            qemu_free(*pse);
+            *pse = next;
+            continue;
+        }
+        pse = &(*pse)->next;
+    }
+}
+
 #define QEMU_VM_FILE_MAGIC           0x5145564d
 #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
 #define QEMU_VM_FILE_VERSION         0x00000003
-- 
1.6.0.6

[Qemu-devel] [PATCH 8/9] Use NICInfo::model for eepro100 savevm ID string

[Mark McLoughlin]

NICInfo::model will always be identical to the device name strings
we're currently passing to nic_init(). Just re-use NICInfo::model.

This makes it clear why we use vc->model for unregister_savevm()
in a subsequent patch.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 hw/eepro100.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 0a343df..c72b990 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1710,15 +1710,14 @@ static void nic_save(QEMUFile * f, void *opaque)
     qemu_put_buffer(f, s->configuration, sizeof(s->configuration));
 }
 
-static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd,
-                     const char *name, uint32_t device)
+static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd, uint32_t device)
 {
     PCIEEPRO100State *d;
     EEPRO100State *s;
 
     logout("\n");
 
-    d = (PCIEEPRO100State *) pci_register_device(bus, name,
+    d = (PCIEEPRO100State *) pci_register_device(bus, nd->model,
                                                  sizeof(PCIEEPRO100State), -1,
                                                  NULL, NULL);
 
@@ -1757,24 +1756,23 @@ static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd,
 
     qemu_register_reset(nic_reset, s);
 
-    register_savevm(name, -1, 3, nic_save, nic_load, s);
+    register_savevm(s->vc->model, -1, 3, nic_save, nic_load, s);
     return (PCIDevice *)d;
 }
 
 PCIDevice *pci_i82551_init(PCIBus * bus, NICInfo * nd, int devfn)
 {
-    return nic_init(bus, nd, "i82551", i82551);
-    //~ uint8_t *pci_conf = d->dev.config;
+    return nic_init(bus, nd, i82551);
 }
 
 PCIDevice *pci_i82557b_init(PCIBus * bus, NICInfo * nd, int devfn)
 {
-    return nic_init(bus, nd, "i82557b", i82557B);
+    return nic_init(bus, nd, i82557B);
 }
 
 PCIDevice *pci_i82559er_init(PCIBus * bus, NICInfo * nd, int devfn)
 {
-    return nic_init(bus, nd, "i82559er", i82559ER);
+    return nic_init(bus, nd, i82559ER);
 }
 
 /* eof */
-- 
1.6.0.6

[Qemu-devel] [PATCH 9/9] Introduce VLANClientState::cleanup()

[Mark McLoughlin]

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

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

diff --git a/hw/e1000.c b/hw/e1000.c
index 2d16774..978f789 100644
--- a/hw/e1000.c
+++ b/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/hw/eepro100.c b/hw/eepro100.c
index c72b990..a54287a 100644
--- a/hw/eepro100.c
+++ b/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->model, s);
+
+    cpu_unregister_io_memory(s->mmio_index);
+
+    eeprom93xx_free(s->eeprom);
+}
+
 static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd, uint32_t device)
 {
     PCIEEPRO100State *d;
@@ -1751,6 +1762,7 @@ static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd, uint32_t device)
 
     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/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
index c87e55f..b0f7f6d 100644
--- a/hw/etraxfs_eth.c
+++ b/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/hw/mcf_fec.c b/hw/mcf_fec.c
index 413c569..6204772 100644
--- a/hw/mcf_fec.c
+++ b/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/hw/mipsnet.c b/hw/mipsnet.c
index 415b04e..f10356a 100644
--- a/hw/mipsnet.c
+++ b/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,10 +258,12 @@ 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;
     if (nd && nd->vlan) {
         s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                      mipsnet_receive, mipsnet_can_receive, s);
+        s->vc->cleanup = mipsnet_cleanup;
     } else {
         s->vc = NULL;
     }
diff --git a/hw/musicpal.c b/hw/musicpal.c
index 5de1691..c4b73eb 100644
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -536,6 +536,7 @@ typedef struct mv88w8618_eth_state {
     uint32_t smir;
     uint32_t icr;
     uint32_t imr;
+    int mmio_index;
     int vlan_header;
     uint32_t tx_queue[2];
     uint32_t rx_queue[4];
@@ -745,10 +746,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");
 
@@ -756,9 +765,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/hw/ne2000.c b/hw/ne2000.c
index 24a66bb..7fe3975 100644
--- a/hw/ne2000.c
+++ b/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/hw/pcnet.c b/hw/pcnet.c
index be68f28..7bd150a 100644
--- a/hw/pcnet.c
+++ b/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);
@@ -1985,6 +1993,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(VLANClientState *vc)
+{
+    PCNetState *d = vc->opaque;
+
+    pcnet_common_cleanup(d);
+
+    cpu_unregister_io_memory(d->mmio_index);
+}
+
 PCIDevice *pci_pcnet_init(PCIBus *bus, NICInfo *nd, int devfn)
 {
     PCNetState *d;
@@ -2032,6 +2049,9 @@ PCIDevice *pci_pcnet_init(PCIBus *bus, NICInfo *nd, int devfn)
     d->pci_dev = &d->dev;
 
     pcnet_common_init(d, nd);
+
+    d->vc->cleanup = pci_pcnet_cleanup;
+
     return (PCIDevice *)d;
 }
 
@@ -2081,29 +2101,44 @@ static CPUWriteMemoryFunc *lance_mem_write[3] = {
     NULL,
 };
 
+static void lance_cleanup(VLANClientState *vc)
+{
+    PCNetState *d = vc->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->vc->cleanup = lance_cleanup;
 }
 #endif /* TARGET_SPARC */
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 9fa69db..e381ab0 100644
--- a/hw/rtl8139.c
+++ b/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/hw/smc91c111.c b/hw/smc91c111.c
index f5b29a7..b132ca3 100644
--- a/hw/smc91c111.c
+++ b/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/hw/stellaris_enet.c b/hw/stellaris_enet.c
index 88c5620..48826d8 100644
--- a/hw/stellaris_enet.c
+++ b/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,23 +385,34 @@ 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);
 
     if (nd->vlan) {
         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);
     }
 
diff --git a/hw/usb-net.c b/hw/usb-net.c
index 863c25f..99c132f 100644
--- a/hw/usb-net.c
+++ b/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/hw/virtio-net.c b/hw/virtio-net.c
index 5e7db0d..201872d 100644
--- a/hw/virtio-net.c
+++ b/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/net.c b/net.c
index 34ec4c8..1378ea8 100644
--- a/net.c
+++ b/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(vc);
             free(vc->name);
             free(vc->model);
             free(vc);
@@ -702,6 +704,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)
 {
@@ -748,6 +752,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,
@@ -760,6 +776,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_handler(s->fd, tap_send, NULL, s);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str), "fd=%d", fd);
@@ -1058,6 +1075,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)
@@ -1079,6 +1104,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));
@@ -1263,6 +1289,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,
@@ -1308,6 +1342,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 */
@@ -1336,6 +1371,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) {
@@ -1895,29 +1931,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/net.h b/net.h
index 1a51be7..5def263 100644
--- a/net.h
+++ b/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/tap-win32.c b/tap-win32.c
index e8a04dc..5948060 100644
--- a/tap-win32.c
+++ b/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

Re: [Qemu-devel] [PATCH 2/9] struct iovec is now universally available

[Christoph Hellwig]

On Wed, Apr 15, 2009 at 05:29:22PM +0100, Mark McLoughlin wrote:
> struct iovec is now defined in qemu-common.h if needed, so we don't need
> the tap code to handle !defined(HAVE_IOVEC).

But is the writev system call also universally available?

[Qemu-devel] Re: [PATCH 5/9] Remove some useless malloc() checking

[Jan Kiszka]

Mark McLoughlin wrote:
> Now that we abort() on malloc, neither qemu_find_vlan() nor
> net_tap_fd_init() can fail.
> 
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> ---
>  net.c |   16 +++-------------
>  1 files changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/net.c b/net.c
> index 2383b10..34ec4c8 100644
> --- a/net.c
> +++ b/net.c
> @@ -1015,8 +1015,6 @@ static int net_tap_init(VLANState *vlan, const char *model,
>  	    return -1;
>      }
>      s = net_tap_fd_init(vlan, model, name, fd);
> -    if (!s)
> -        return -1;
>      snprintf(s->vc->info_str, sizeof(s->vc->info_str),
>               "ifname=%s,script=%s,downscript=%s",
>               ifname, setup_script, down_script);
> @@ -1596,10 +1594,7 @@ int net_client_init(const char *device, const char *p)
>          vlan_id = strtol(buf, NULL, 0);
>      }
>      vlan = qemu_find_vlan(vlan_id);

Maybe it would be a good idea to rename this function at this chance:
qemu_get_vlan. For a while I was thinking that it only finds what is
already there...

> -    if (!vlan) {
> -        fprintf(stderr, "Could not create vlan %d\n", vlan_id);
> -        return -1;
> -    }
> +
>      if (get_param_value(buf, sizeof(buf), "name", p)) {
>          name = strdup(buf);
>      }
> @@ -1707,9 +1702,8 @@ int net_client_init(const char *device, const char *p)
>          if (get_param_value(buf, sizeof(buf), "fd", p) > 0) {
>              fd = strtol(buf, NULL, 0);
>              fcntl(fd, F_SETFL, O_NONBLOCK);
> -            ret = -1;
> -            if (net_tap_fd_init(vlan, device, name, fd))
> -                ret = 0;
> +            net_tap_fd_init(vlan, device, name, fd);
> +            ret = 0;
>          } else {
>              if (get_param_value(ifname, sizeof(ifname), "ifname", p) <= 0) {
>                  ifname[0] = '\0';
> @@ -1825,10 +1819,6 @@ void net_host_device_remove(Monitor *mon, int vlan_id, const char *device)
>      VLANClientState *vc;
>  
>      vlan = qemu_find_vlan(vlan_id);
> -    if (!vlan) {
> -        monitor_printf(mon, "can't find vlan %d\n", vlan_id);
> -        return;
> -    }
>  
>     for(vc = vlan->first_client; vc != NULL; vc = vc->next)
>          if (!strcmp(vc->name, device))

Looks good.

Jan

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

Re: [Qemu-devel] [PATCH 2/9] struct iovec is now universally available

[Mark McLoughlin]

On Wed, 2009-04-15 at 18:55 +0200, Christoph Hellwig wrote:
> On Wed, Apr 15, 2009 at 05:29:22PM +0100, Mark McLoughlin wrote:
> > struct iovec is now defined in qemu-common.h if needed, so we don't need
> > the tap code to handle !defined(HAVE_IOVEC).
> 
> But is the writev system call also universally available?

Fair point.

I don't know of anywhere that qemu currently builds where writev() isn't
available ... do you?

>From looking at the history, I got the impression that HAVE_IOVEC was
only for windows and that doesn't use this code.

Also, if it is something we want to handle, then we should do it by
checking for writev() in configure and using HAVE_WRITEV

Cheers,
Mark.

Re: [Qemu-devel] [PATCH 2/9] struct iovec is now universally available

[Christoph Hellwig]

On Wed, Apr 15, 2009 at 06:25:09PM +0100, Mark McLoughlin wrote:
> On Wed, 2009-04-15 at 18:55 +0200, Christoph Hellwig wrote:
> > On Wed, Apr 15, 2009 at 05:29:22PM +0100, Mark McLoughlin wrote:
> > > struct iovec is now defined in qemu-common.h if needed, so we don't need
> > > the tap code to handle !defined(HAVE_IOVEC).
> > 
> > But is the writev system call also universally available?
> 
> Fair point.
> 
> I don't know of anywhere that qemu currently builds where writev() isn't
> available ... do you?
>
> >From looking at the history, I got the impression that HAVE_IOVEC was
> only for windows and that doesn't use this code.

Windows would be my first guess, but if that code isn't built for
windows the point is moot.

> Also, if it is something we want to handle, then we should do it by
> checking for writev() in configure and using HAVE_WRITEV

Agreed.

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Jan Kiszka]

Mark McLoughlin wrote:
> We're currently leaking memory and file descriptors on device
> hot-unplug.
> 
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> ---
>  hw/e1000.c          |   12 +++++-----
>  hw/eepro100.c       |   12 ++++++++++
>  hw/etraxfs_eth.c    |   11 +++++++++
>  hw/mcf_fec.c        |   18 ++++++++++++---
>  hw/mipsnet.c        |   14 ++++++++++++
>  hw/musicpal.c       |   18 ++++++++++++---
>  hw/ne2000.c         |   24 +++++++++++++++++++++
>  hw/pcnet.c          |   43 ++++++++++++++++++++++++++++++++++---
>  hw/rtl8139.c        |   20 +++++++++++++++++
>  hw/smc91c111.c      |   17 +++++++++++---
>  hw/stellaris_enet.c |   20 ++++++++++++++---
>  hw/usb-net.c        |   11 ++++++++-
>  hw/virtio-net.c     |   14 ++++++++++++
>  net.c               |   57 +++++++++++++++++++++++++++++++++++++-------------
>  net.h               |    2 +
>  tap-win32.c         |   13 +++++++++++
>  16 files changed, 263 insertions(+), 43 deletions(-)
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 2d16774..978f789 100644
> --- a/hw/e1000.c
> +++ b/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;

Just to leave my comment here as well :) : I still consider this an
important, mostly required callback that should be lifted into
qemu_new_vlan_client(). That way, everyone who thinks (s)he doesn't need
it will have to explicitly null'ify it.

>      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/hw/eepro100.c b/hw/eepro100.c
> index c72b990..a54287a 100644
> --- a/hw/eepro100.c
> +++ b/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->model, s);
> +
> +    cpu_unregister_io_memory(s->mmio_index);
> +
> +    eeprom93xx_free(s->eeprom);
> +}
> +
>  static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd, uint32_t device)
>  {
>      PCIEEPRO100State *d;
> @@ -1751,6 +1762,7 @@ static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd, uint32_t device)
>  
>      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/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
> index c87e55f..b0f7f6d 100644
> --- a/hw/etraxfs_eth.c
> +++ b/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/hw/mcf_fec.c b/hw/mcf_fec.c
> index 413c569..6204772 100644
> --- a/hw/mcf_fec.c
> +++ b/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/hw/mipsnet.c b/hw/mipsnet.c
> index 415b04e..f10356a 100644
> --- a/hw/mipsnet.c
> +++ b/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,10 +258,12 @@ 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;
>      if (nd && nd->vlan) {
>          s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
>                                       mipsnet_receive, mipsnet_can_receive, s);
> +        s->vc->cleanup = mipsnet_cleanup;
>      } else {
>          s->vc = NULL;
>      }
> diff --git a/hw/musicpal.c b/hw/musicpal.c
> index 5de1691..c4b73eb 100644
> --- a/hw/musicpal.c
> +++ b/hw/musicpal.c
> @@ -536,6 +536,7 @@ typedef struct mv88w8618_eth_state {
>      uint32_t smir;
>      uint32_t icr;
>      uint32_t imr;
> +    int mmio_index;
>      int vlan_header;
>      uint32_t tx_queue[2];
>      uint32_t rx_queue[4];
> @@ -745,10 +746,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");
>  
> @@ -756,9 +765,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/hw/ne2000.c b/hw/ne2000.c
> index 24a66bb..7fe3975 100644
> --- a/hw/ne2000.c
> +++ b/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);
> +}

Hot-plugging ISA device - scary... :)

> +
>  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/hw/pcnet.c b/hw/pcnet.c
> index be68f28..7bd150a 100644
> --- a/hw/pcnet.c
> +++ b/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);
> @@ -1985,6 +1993,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(VLANClientState *vc)
> +{
> +    PCNetState *d = vc->opaque;
> +
> +    pcnet_common_cleanup(d);
> +
> +    cpu_unregister_io_memory(d->mmio_index);
> +}
> +
>  PCIDevice *pci_pcnet_init(PCIBus *bus, NICInfo *nd, int devfn)
>  {
>      PCNetState *d;
> @@ -2032,6 +2049,9 @@ PCIDevice *pci_pcnet_init(PCIBus *bus, NICInfo *nd, int devfn)
>      d->pci_dev = &d->dev;
>  
>      pcnet_common_init(d, nd);
> +
> +    d->vc->cleanup = pci_pcnet_cleanup;
> +
>      return (PCIDevice *)d;
>  }
>  
> @@ -2081,29 +2101,44 @@ static CPUWriteMemoryFunc *lance_mem_write[3] = {
>      NULL,
>  };
>  
> +static void lance_cleanup(VLANClientState *vc)
> +{
> +    PCNetState *d = vc->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->vc->cleanup = lance_cleanup;
>  }
>  #endif /* TARGET_SPARC */
> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> index 9fa69db..e381ab0 100644
> --- a/hw/rtl8139.c
> +++ b/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/hw/smc91c111.c b/hw/smc91c111.c
> index f5b29a7..b132ca3 100644
> --- a/hw/smc91c111.c
> +++ b/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/hw/stellaris_enet.c b/hw/stellaris_enet.c
> index 88c5620..48826d8 100644
> --- a/hw/stellaris_enet.c
> +++ b/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,23 +385,34 @@ 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);
>  
>      if (nd->vlan) {
>          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);
>      }
>  
> diff --git a/hw/usb-net.c b/hw/usb-net.c
> index 863c25f..99c132f 100644
> --- a/hw/usb-net.c
> +++ b/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/hw/virtio-net.c b/hw/virtio-net.c
> index 5e7db0d..201872d 100644
> --- a/hw/virtio-net.c
> +++ b/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/net.c b/net.c
> index 34ec4c8..1378ea8 100644
> --- a/net.c
> +++ b/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(vc);
>              free(vc->name);
>              free(vc->model);
>              free(vc);

That should become qemu_free(vc) at this chance. The rest should be
strdup-allocated.

> @@ -702,6 +704,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)
>  {
> @@ -748,6 +752,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);

free -> qemu_free

> +}
> +
>  /* fd support */
>  
>  static TAPState *net_tap_fd_init(VLANState *vlan,
> @@ -760,6 +776,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_handler(s->fd, tap_send, NULL, s);
>      snprintf(s->vc->info_str, sizeof(s->vc->info_str), "fd=%d", fd);
> @@ -1058,6 +1075,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);

same here

> +}
> +
>  static int net_vde_init(VLANState *vlan, const char *model,
>                          const char *name, const char *sock,
>                          int port, const char *group, int mode)
> @@ -1079,6 +1104,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));
> @@ -1263,6 +1289,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);

and here

> +}
> +
>  static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan,
>                                                  const char *model,
>                                                  const char *name,
> @@ -1308,6 +1342,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 */
> @@ -1336,6 +1371,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) {
> @@ -1895,29 +1931,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/net.h b/net.h
> index 1a51be7..5def263 100644
> --- a/net.h
> +++ b/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/tap-win32.c b/tap-win32.c
> index e8a04dc..5948060 100644
> --- a/tap-win32.c
> +++ b/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);

also here: qemu_free

> +}
> +
>  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);

Besides the remarks: looks better than mine.

Jan

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

Re: [Qemu-devel] [PATCH 2/9] struct iovec is now universally available

[M. Warner Losh]

In message: <20090415165513.GA14504@lst.de>
            Christoph Hellwig <hch@lst.de> writes:
: On Wed, Apr 15, 2009 at 05:29:22PM +0100, Mark McLoughlin wrote:
: > struct iovec is now defined in qemu-common.h if needed, so we don't need
: > the tap code to handle !defined(HAVE_IOVEC).
: 
: But is the writev system call also universally available?

>From FreeBSD's writev man page:

The writev() system call appeared in 4.2BSD.  The write() function
appeared in Version 6 AT&T UNIX.

Warner

Re: [Qemu-devel] [PATCH 2/9] struct iovec is now universally available

[François Revol]

> On Wed, 2009-04-15 at 18:55 +0200, Christoph Hellwig wrote:
> > On Wed, Apr 15, 2009 at 05:29:22PM +0100, Mark McLoughlin wrote:
> > > struct iovec is now defined in qemu-common.h if needed, so we
> > > don't need
> > > the tap code to handle !defined(HAVE_IOVEC).
> >
> > But is the writev system call also universally available?
>
> Fair point.
>
> I don't know of anywhere that qemu currently builds where writev()
> isn't
> available ... do you?

Even BeOS has writev :P

François.

Re: [Qemu-devel] [PATCH 2/9] struct iovec is now universally available

[M. Warner Losh]

In message: <6254906245-BeMail@laptop>
            "François Revol" <revol@free.fr> writes:
: > On Wed, 2009-04-15 at 18:55 +0200, Christoph Hellwig wrote:
: > > On Wed, Apr 15, 2009 at 05:29:22PM +0100, Mark McLoughlin wrote:
: > > > struct iovec is now defined in qemu-common.h if needed, so we 
: > > > don't need
: > > > the tap code to handle !defined(HAVE_IOVEC).
: > > 
: > > But is the writev system call also universally available?
: > 
: > Fair point.
: > 
: > I don't know of anywhere that qemu currently builds where writev() 
: > isn't
: > available ... do you?
: 
: Even BeOS has writev :P

writev(2) was used in the 4.2BSD network daemons, so it tended to be
picked up by everybody that picked up the BSD TCP network
stack/interface.

Warner

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Marcelo Tosatti]

Hi Mark,

Nice!

On Wed, Apr 15, 2009 at 07:34:21PM +0200, Jan Kiszka wrote:
> Mark McLoughlin wrote:
> > We're currently leaking memory and file descriptors on device
> > hot-unplug.
> > 
> > Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> > ---
> >  hw/e1000.c          |   12 +++++-----
> >  hw/eepro100.c       |   12 ++++++++++
> >  hw/etraxfs_eth.c    |   11 +++++++++
> >  hw/mcf_fec.c        |   18 ++++++++++++---
> >  hw/mipsnet.c        |   14 ++++++++++++
> >  hw/musicpal.c       |   18 ++++++++++++---
> >  hw/ne2000.c         |   24 +++++++++++++++++++++
> >  hw/pcnet.c          |   43 ++++++++++++++++++++++++++++++++++---
> >  hw/rtl8139.c        |   20 +++++++++++++++++
> >  hw/smc91c111.c      |   17 +++++++++++---
> >  hw/stellaris_enet.c |   20 ++++++++++++++---
> >  hw/usb-net.c        |   11 ++++++++-
> >  hw/virtio-net.c     |   14 ++++++++++++
> >  net.c               |   57 +++++++++++++++++++++++++++++++++++++-------------
> >  net.h               |    2 +
> >  tap-win32.c         |   13 +++++++++++
> >  16 files changed, 263 insertions(+), 43 deletions(-)
> > 
> > diff --git a/hw/e1000.c b/hw/e1000.c
> > index 2d16774..978f789 100644
> > --- a/hw/e1000.c
> > +++ b/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;
> 
> Just to leave my comment here as well :) : I still consider this an
> important, mostly required callback that should be lifted into
> qemu_new_vlan_client(). That way, everyone who thinks (s)he doesn't need
> it will have to explicitly null'ify it.

Agreed.

> 
> >      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;

I'm unsure about the fact that you consider device dependant details
such as MMIO addresses part of the "VLANClient" abstraction. Don't 
they belong to the PCI device, and as such, should be unregistered
in (PCIDevice *)->unregister?

> > +static void mcf_fec_cleanup(VLANClientState *vc)
> > +{
> > +    mcf_fec_state *s = vc->opaque;
> > +
> > +    cpu_unregister_io_memory(s->mmio_index);
> > +
> > +    qemu_free(s);
> > +}

Also the fact that you free the device structure in the non-PCI
functions, but you don't in the PCI functions (because generic PCI
code does it) is somewhat confusing.

Hum, I think abstracting away ISA devices would be a good thing.

> > +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);
> > +}
> Hot-plugging ISA device - scary... :)

Yeah :)

> > +static void ne2000_cleanup(VLANClientState *vc)
> > +{
> > +    NE2000State *s = vc->opaque;
> > +
> > +    unregister_savevm("ne2000", s);
> > +}

So unregister_savevm is common to all buses for the ne2000 chip, but
isa_unassign_ioport is not. So what about moving non-device specific
details to (VLANClientState *)->cleanup, and device specific to
(XXXDevice *)->unregister?

For example there was symmetry between lsi_scsi_unregister and
e1000_unregister before.

This would make the purpose of the interface you are creating clearer,
IMHO.

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[M. Warner Losh]

In message: <20090416010725.GA24264@amt.cnet>
            Marcelo Tosatti <mtosatti@redhat.com> writes:
: > > +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);
: > > +}
: > Hot-plugging ISA device - scary... :)
: 
: Yeah :)

No it isn't.  They called it PC Card (aka PCMCIA) back in the day :)

Warner

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Mark McLoughlin]

Hi Jan,

On Wed, 2009-04-15 at 19:34 +0200, Jan Kiszka wrote:
> Mark McLoughlin wrote:

> > @@ -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;
> 
> Just to leave my comment here as well :) : I still consider this an
> important, mostly required callback that should be lifted into
> qemu_new_vlan_client(). That way, everyone who thinks (s)he doesn't need
> it will have to explicitly null'ify it.

I still think this is terribly ugly, but I'll add the argument to
new_vlan_client() until I can fix the ugliness more comprehensively :-)

>       qemu_format_nic_info_str(n->vc, n->mac);
> > diff --git a/net.c b/net.c
> > index 34ec4c8..1378ea8 100644
> > --- a/net.c
> > +++ b/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(vc);
> >              free(vc->name);
> >              free(vc->model);
> >              free(vc);
> 
> That should become qemu_free(vc) at this chance. The rest should be
> strdup-allocated.

I'll queue that up as a separate patch.

...
> > @@ -748,6 +752,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);
> 
> free -> qemu_free

Thanks; I had spotted this and forgot to fix it before posting.

Cheers,
Mark.

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Mark McLoughlin]

Hi Marcelo,

On Wed, 2009-04-15 at 22:07 -0300, Marcelo Tosatti wrote:
> On Wed, Apr 15, 2009 at 07:34:21PM +0200, Jan Kiszka wrote:
> > Mark McLoughlin wrote:
> > > @@ -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;
> > 
> > Just to leave my comment here as well :) : I still consider this an
> > important, mostly required callback that should be lifted into
> > qemu_new_vlan_client(). That way, everyone who thinks (s)he doesn't need
> > it will have to explicitly null'ify it.
> 
> Agreed.

Oi! You're the one that introduced this:

    d->dev.unregister = pci_e1000_uninit;

which is basically the same thing. But ... "whatever" :-)

> > >      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;
> 
> I'm unsure about the fact that you consider device dependant details
> such as MMIO addresses part of the "VLANClient" abstraction. Don't 
> they belong to the PCI device, and as such, should be unregistered
> in (PCIDevice *)->unregister?
> 
> > > +static void mcf_fec_cleanup(VLANClientState *vc)
> > > +{
> > > +    mcf_fec_state *s = vc->opaque;
> > > +
> > > +    cpu_unregister_io_memory(s->mmio_index);
> > > +
> > > +    qemu_free(s);
> > > +}
> 
> Also the fact that you free the device structure in the non-PCI
> functions, but you don't in the PCI functions (because generic PCI
> code does it) is somewhat confusing.
> 
> Hum, I think abstracting away ISA devices would be a good thing.
> 
...
> > > +static void ne2000_cleanup(VLANClientState *vc)
> > > +{
> > > +    NE2000State *s = vc->opaque;
> > > +
> > > +    unregister_savevm("ne2000", s);
> > > +}
> 
> So unregister_savevm is common to all buses for the ne2000 chip, but
> isa_unassign_ioport is not. So what about moving non-device specific
> details to (VLANClientState *)->cleanup, and device specific to
> (XXXDevice *)->unregister?
> 
> For example there was symmetry between lsi_scsi_unregister and
> e1000_unregister before.
> 
> This would make the purpose of the interface you are creating clearer,
> IMHO.

I see where you're coming from, especially with the symmetry with block
devices.

However, the way I see it is that the VLANClientState should "own" the
PCIDevice, not the other way around - e.g. you want to free the device,
you should do qemu_del_vlan_client(), rather than
pci_device_unregister(). 

What follows from that is VLANClientState::cleanup() should call
pci_device_unregister().

If we did it the other way, then PCIDevice::unregister() should do
qemu_del_vlan_client() and callers should never free a PCI NIC directly
using del_vlan_client(), but instead call pci_device_unregister().

Futhermore, if we took the latter approach, we'd need a similar
abstraction to PCIDevice for the non-PCI NICs.

As for splitting the cleanups non-device specific and device specific
parts ... for most devices, no such separation exists. We mix all the
state up in the structure, and it's all allocated in the one place, so
separating out the cleanup seems a bit arbitrary.

Following up with an updated version of the patch which uses
PCIDevice::unregister() for unregister I/O memory and
VLANClientState::cleanup() for cleaning everything else up.

It's not perfect, by any means ... but it's a baby step in the right
direction IMHO.

Cheers,
Mark.

[Qemu-devel] [PATCH 09/09 v2] Introduce VLANClientState::cleanup()

[Mark McLoughlin]

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

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 hw/dp8393x.c        |   20 +++++++++++---
 hw/e1000.c          |   11 +++++++-
 hw/eepro100.c       |   23 ++++++++++++++++-
 hw/etraxfs_eth.c    |   13 ++++++++-
 hw/mcf_fec.c        |   20 +++++++++++---
 hw/mipsnet.c        |   16 +++++++++++-
 hw/musicpal.c       |   20 +++++++++++---
 hw/ne2000.c         |   28 ++++++++++++++++++-
 hw/pcnet.c          |   58 +++++++++++++++++++++++++++++++++++------
 hw/rtl8139.c        |   31 +++++++++++++++++++++-
 hw/smc91c111.c      |   19 ++++++++++---
 hw/stellaris_enet.c |   23 +++++++++++++---
 hw/usb-net.c        |   14 ++++++++--
 hw/virtio-net.c     |   19 +++++++++++++-
 hw/virtio.c         |    7 +++++
 hw/virtio.h         |    2 +
 net.c               |   71 ++++++++++++++++++++++++++++++++++++---------------
 net.h               |    3 ++
 tap-win32.c         |   15 ++++++++++-
 19 files changed, 347 insertions(+), 66 deletions(-)

diff --git a/hw/dp8393x.c b/hw/dp8393x.c
index eed6eeb..6170588 100644
--- a/hw/dp8393x.c
+++ b/hw/dp8393x.c
@@ -156,6 +156,7 @@ typedef struct dp8393xState {
     QEMUTimer *watchdog;
     int64_t wt_last_update;
     VLANClientState *vc;
+    int mmio_index;
 
     /* Registers */
     uint8_t cam[16][6];
@@ -858,12 +859,23 @@ static void nic_reset(void *opaque)
     dp8393x_update_irq(s);
 }
 
+static void nic_cleanup(VLANClientState *vc)
+{
+    dp8393xState *s = vc->opaque;
+
+    cpu_unregister_io_memory(s->mmio_index);
+
+    qemu_del_timer(s->watchdog);
+    qemu_free_timer(s->watchdog);
+
+    qemu_free(s);
+}
+
 void dp83932_init(NICInfo *nd, target_phys_addr_t base, int it_shift,
                   qemu_irq irq, void* mem_opaque,
                   void (*memory_rw)(void *opaque, target_phys_addr_t addr, uint8_t *buf, int len, int is_write))
 {
     dp8393xState *s;
-    int io;
 
     qemu_check_nic_model(nd, "dp83932");
 
@@ -877,12 +889,12 @@ void dp83932_init(NICInfo *nd, target_phys_addr_t base, int it_shift,
     s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
 
     s->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
-                                 nic_receive, nic_can_receive, s);
+                                 nic_receive, nic_can_receive, nic_cleanup, s);
 
     qemu_format_nic_info_str(s->vc, nd->macaddr);
     qemu_register_reset(nic_reset, s);
     nic_reset(s);
 
-    io = cpu_register_io_memory(0, dp8393x_read, dp8393x_write, s);
-    cpu_register_physical_memory(base, 0x40 << it_shift, io);
+    s->mmio_index = cpu_register_io_memory(0, dp8393x_read, dp8393x_write, s);
+    cpu_register_physical_memory(base, 0x40 << it_shift, s->mmio_index);
 }
diff --git a/hw/e1000.c b/hw/e1000.c
index 2d16774..1729db2 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1033,6 +1033,14 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
                                      excluded_regs[i] - 4);
 }
 
+static void
+e1000_cleanup(VLANClientState *vc)
+{
+    E1000State *d = vc->opaque;
+
+    unregister_savevm("e1000", d);
+}
+
 static int
 pci_e1000_uninit(PCIDevice *dev)
 {
@@ -1094,7 +1102,8 @@ 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,
+                                 e1000_cleanup, d);
     d->vc->link_status_changed = e1000_set_link_status;
 
     qemu_format_nic_info_str(d->vc, nd->macaddr);
diff --git a/hw/eepro100.c b/hw/eepro100.c
index c72b990..18d8115 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1710,6 +1710,25 @@ 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->model, s);
+
+    eeprom93xx_free(s->eeprom);
+}
+
+static int pci_nic_uninit(PCIDevice *dev)
+{
+    PCIEEPRO100State *d = (PCIEEPRO100State *) dev;
+    EEPRO100State *s = &d->eepro100;
+
+    cpu_unregister_io_memory(s->mmio_index);
+
+    return 0;
+}
+
 static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd, uint32_t device)
 {
     PCIEEPRO100State *d;
@@ -1720,6 +1739,7 @@ static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd, uint32_t device)
     d = (PCIEEPRO100State *) pci_register_device(bus, nd->model,
                                                  sizeof(PCIEEPRO100State), -1,
                                                  NULL, NULL);
+    d->dev.unregister = pci_nic_uninit;
 
     s = &d->eepro100;
     s->device = device;
@@ -1750,7 +1770,8 @@ static PCIDevice *nic_init(PCIBus * bus, NICInfo * nd, uint32_t device)
     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,
+                                 nic_cleanup, s);
 
     qemu_format_nic_info_str(s->vc, s->macaddr);
 
diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
index c87e55f..15270f5 100644
--- a/hw/etraxfs_eth.c
+++ b/hw/etraxfs_eth.c
@@ -554,6 +554,16 @@ static CPUWriteMemoryFunc *eth_write[] = {
 	&eth_writel,
 };
 
+static void 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)
 {
@@ -585,7 +595,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,
+				       eth_cleanup, 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..1ca847b 100644
--- a/hw/mcf_fec.c
+++ b/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);
+                                 mcf_fec_receive, mcf_fec_can_receive,
+                                 mcf_fec_cleanup, 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 415b04e..e842984 100644
--- a/hw/mipsnet.c
+++ b/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,10 +258,12 @@ 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;
     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,
+                                     mipsnet_cleanup, s);
     } else {
         s->vc = NULL;
     }
diff --git a/hw/musicpal.c b/hw/musicpal.c
index 5de1691..fc227e9 100644
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -536,6 +536,7 @@ typedef struct mv88w8618_eth_state {
     uint32_t smir;
     uint32_t icr;
     uint32_t imr;
+    int mmio_index;
     int vlan_header;
     uint32_t tx_queue[2];
     uint32_t rx_queue[4];
@@ -745,20 +746,29 @@ static CPUWriteMemoryFunc *mv88w8618_eth_writefn[] = {
     mv88w8618_eth_write
 };
 
+static void 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");
 
     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);
-    iomemtype = cpu_register_io_memory(0, mv88w8618_eth_readfn,
-                                       mv88w8618_eth_writefn, s);
-    cpu_register_physical_memory(base, MP_ETH_SIZE, iomemtype);
+                                 eth_receive, eth_can_receive,
+                                 eth_cleanup, s);
+    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/hw/ne2000.c b/hw/ne2000.c
index 24a66bb..99612e2 100644
--- a/hw/ne2000.c
+++ b/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,13 +750,15 @@ 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);
 
     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,
+                                 isa_ne2000_cleanup, s);
 
     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;
@@ -802,7 +825,8 @@ 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,
+                                 ne2000_cleanup, s);
 
     qemu_format_nic_info_str(s->vc, s->macaddr);
 
diff --git a/hw/pcnet.c b/hw/pcnet.c
index be68f28..acbaee6 100644
--- a/hw/pcnet.c
+++ b/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,7 +1930,15 @@ static int pcnet_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
-static void pcnet_common_init(PCNetState *d, NICInfo *nd)
+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, NetCleanup *cleanup)
 {
     d->poll_timer = qemu_new_timer(vm_clock, pcnet_poll_timer, d);
 
@@ -1937,7 +1946,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,
+                                     cleanup, d);
 
         qemu_format_nic_info_str(d->vc, d->nd->macaddr);
     } else {
@@ -1985,6 +1995,22 @@ 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(VLANClientState *vc)
+{
+    PCNetState *d = vc->opaque;
+
+    pcnet_common_cleanup(d);
+}
+
+static int pci_pcnet_uninit(PCIDevice *dev)
+{
+    PCNetState *d = (PCNetState *)dev;
+
+    cpu_unregister_io_memory(d->mmio_index);
+
+    return 0;
+}
+
 PCIDevice *pci_pcnet_init(PCIBus *bus, NICInfo *nd, int devfn)
 {
     PCNetState *d;
@@ -1997,7 +2023,7 @@ PCIDevice *pci_pcnet_init(PCIBus *bus, NICInfo *nd, int devfn)
 
     d = (PCNetState *)pci_register_device(bus, "PCNet", sizeof(PCNetState),
                                           devfn, NULL, NULL);
-
+    d->dev.unregister = pci_pcnet_uninit;
     pci_conf = d->dev.config;
 
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_AMD);
@@ -2031,7 +2057,8 @@ PCIDevice *pci_pcnet_init(PCIBus *bus, NICInfo *nd, int devfn)
     d->phys_mem_write = pci_physical_memory_write;
     d->pci_dev = &d->dev;
 
-    pcnet_common_init(d, nd);
+    pcnet_common_init(d, nd, pci_pcnet_cleanup);
+
     return (PCIDevice *)d;
 }
 
@@ -2081,29 +2108,42 @@ static CPUWriteMemoryFunc *lance_mem_write[3] = {
     NULL,
 };
 
+static void lance_cleanup(VLANClientState *vc)
+{
+    PCNetState *d = vc->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);
+    pcnet_common_init(d, nd, lance_cleanup);
 }
 #endif /* TARGET_SPARC */
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 9fa69db..0093ff4 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -3414,6 +3414,33 @@ 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);
+}
+
+static int pci_rtl8139_uninit(PCIDevice *dev)
+{
+    PCIRTL8139State *d = (PCIRTL8139State *)dev;
+    RTL8139State *s = &d->rtl8139;
+
+    cpu_unregister_io_memory(s->rtl8139_mmio_io_addr);
+
+    return 0;
+}
+
 PCIDevice *pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
 {
     PCIRTL8139State *d;
@@ -3424,6 +3451,7 @@ PCIDevice *pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
                                               "RTL8139", sizeof(PCIRTL8139State),
                                               devfn,
                                               NULL, NULL);
+    d->dev.unregister = pci_rtl8139_uninit;
     pci_conf = d->dev.config;
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_REALTEK);
     pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_REALTEK_8139);
@@ -3450,7 +3478,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,
+                                 rtl8139_cleanup, s);
 
     qemu_format_nic_info_str(s->vc, s->macaddr);
 
diff --git a/hw/smc91c111.c b/hw/smc91c111.c
index f5b29a7..9f567ab 100644
--- a/hw/smc91c111.c
+++ b/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,24 +691,32 @@ 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);
 
     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,
+                                 smc91c111_cleanup, 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..a4c2011 100644
--- a/hw/stellaris_enet.c
+++ b/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,23 +385,35 @@ 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);
 
     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,
+                                     stellaris_enet_cleanup, s);
         qemu_format_nic_info_str(s->vc, s->macaddr);
     }
 
diff --git a/hw/usb-net.c b/hw/usb-net.c
index 863c25f..9e64425 100644
--- a/hw/usb-net.c
+++ b/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)
@@ -1452,7 +1458,9 @@ 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,
+                                 usbnet_cleanup, s);
 
     qemu_format_nic_info_str(s->vc, s->mac);
 
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 5e7db0d..f9717c0 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -570,6 +570,21 @@ 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);
+
+    virtio_cleanup(&n->vdev);
+}
+
 PCIDevice *virtio_net_init(PCIBus *bus, NICInfo *nd, int devfn)
 {
     VirtIONet *n;
@@ -598,7 +613,9 @@ 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,
+                                 virtio_net_cleanup, n);
     n->vc->link_status_changed = virtio_net_set_link_status;
 
     qemu_format_nic_info_str(n->vc, n->mac);
diff --git a/hw/virtio.c b/hw/virtio.c
index 93a7de6..4aa5f20 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -750,6 +750,13 @@ void virtio_load(VirtIODevice *vdev, QEMUFile *f)
     virtio_update_irq(vdev);
 }
 
+void virtio_cleanup(VirtIODevice *vdev)
+{
+    if (vdev->config)
+        qemu_free(vdev->config);
+    qemu_free(vdev->vq);
+}
+
 VirtIODevice *virtio_init_pci(PCIBus *bus, const char *name,
                               uint16_t vendor, uint16_t device,
                               uint16_t subvendor, uint16_t subdevice,
diff --git a/hw/virtio.h b/hw/virtio.h
index cce8a47..935b118 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -117,6 +117,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f);
 
 void virtio_load(VirtIODevice *vdev, QEMUFile *f);
 
+void virtio_cleanup(VirtIODevice *vdev);
+
 void virtio_notify_config(VirtIODevice *vdev);
 
 void virtio_queue_set_notification(VirtQueue *vq, int enable);
diff --git a/net.c b/net.c
index 34ec4c8..5a8f824 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,
+                                      NetCleanup *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);
+            }
             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;
 }
@@ -702,6 +707,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)
 {
@@ -748,6 +755,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);
+    qemu_free(s);
+}
+
 /* fd support */
 
 static TAPState *net_tap_fd_init(VLANState *vlan,
@@ -759,7 +778,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, tap_cleanup, s);
     s->vc->fd_readv = tap_receive_iov;
     qemu_set_fd_handler(s->fd, tap_send, NULL, s);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str), "fd=%d", fd);
@@ -1058,6 +1078,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);
+    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)
@@ -1078,7 +1106,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, 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));
@@ -1263,6 +1292,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);
+    qemu_free(s);
+}
+
 static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan,
                                                 const char *model,
                                                 const char *name,
@@ -1307,7 +1344,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 */
@@ -1334,8 +1372,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) {
@@ -1895,29 +1933,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/net.h b/net.h
index 1a51be7..413f705 100644
--- a/net.h
+++ b/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;
@@ -40,6 +42,7 @@ VLANClientState *qemu_new_vlan_client(VLANState *vlan,
                                       const char *name,
                                       IOReadHandler *fd_read,
                                       IOCanRWHandler *fd_can_read,
+                                      NetCleanup *cleanup,
                                       void *opaque);
 void qemu_del_vlan_client(VLANClientState *vc);
 VLANClientState *qemu_find_vlan_client(VLANState *vlan, void *opaque);
diff --git a/tap-win32.c b/tap-win32.c
index e8a04dc..3ff957f 100644
--- a/tap-win32.c
+++ b/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);
+    */
+    qemu_free(s);
+}
+
 static void tap_receive(void *opaque, const uint8_t *buf, int size)
 {
     TAPState *s = opaque;
@@ -672,7 +684,8 @@ int tap_win32_init(VLANState *vlan, const char *model,
         return -1;
     }
 
-    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, tap_cleanup, s);
 
     snprintf(s->vc->info_str, sizeof(s->vc->info_str),
              "tap: ifname=%s", ifname);
-- 
1.6.0.6

[Qemu-devel] [PATCH 10/09] Free VLANClientState using qemu_free()

[Mark McLoughlin]

It's allocated using qemu_mallocz(), so ...

The name and model strings are strdup() allocated, so free()
is still appropriate for them.

Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net.c b/net.c
index 5a8f824..3cd6277 100644
--- a/net.c
+++ b/net.c
@@ -369,7 +369,7 @@ void qemu_del_vlan_client(VLANClientState *vc)
             }
             free(vc->name);
             free(vc->model);
-            free(vc);
+            qemu_free(vc);
             break;
         } else
             pvc = &(*pvc)->next;
-- 
1.6.0.6

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Marcelo Tosatti]

On Thu, Apr 16, 2009 at 03:49:57PM +0100, Mark McLoughlin wrote:
> Hi Marcelo,
> 
> On Wed, 2009-04-15 at 22:07 -0300, Marcelo Tosatti wrote:
> > On Wed, Apr 15, 2009 at 07:34:21PM +0200, Jan Kiszka wrote:
> > > Mark McLoughlin wrote:
> > > > @@ -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;
> > > 
> > > Just to leave my comment here as well :) : I still consider this an
> > > important, mostly required callback that should be lifted into
> > > qemu_new_vlan_client(). That way, everyone who thinks (s)he doesn't need
> > > it will have to explicitly null'ify it.
> > 
> > Agreed.
> 
> Oi! You're the one that introduced this:
> 
>     d->dev.unregister = pci_e1000_uninit;
> 
> which is basically the same thing. But ... "whatever" :-)

> > > >      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;
> > 
> > I'm unsure about the fact that you consider device dependant details
> > such as MMIO addresses part of the "VLANClient" abstraction. Don't 
> > they belong to the PCI device, and as such, should be unregistered
> > in (PCIDevice *)->unregister?
> > 
> > > > +static void mcf_fec_cleanup(VLANClientState *vc)
> > > > +{
> > > > +    mcf_fec_state *s = vc->opaque;
> > > > +
> > > > +    cpu_unregister_io_memory(s->mmio_index);
> > > > +
> > > > +    qemu_free(s);
> > > > +}
> > 
> > Also the fact that you free the device structure in the non-PCI
> > functions, but you don't in the PCI functions (because generic PCI
> > code does it) is somewhat confusing.
> > 
> > Hum, I think abstracting away ISA devices would be a good thing.
> > 
> ...
> > > > +static void ne2000_cleanup(VLANClientState *vc)
> > > > +{
> > > > +    NE2000State *s = vc->opaque;
> > > > +
> > > > +    unregister_savevm("ne2000", s);
> > > > +}
> > 
> > So unregister_savevm is common to all buses for the ne2000 chip, but
> > isa_unassign_ioport is not. So what about moving non-device specific
> > details to (VLANClientState *)->cleanup, and device specific to
> > (XXXDevice *)->unregister?
> > 
> > For example there was symmetry between lsi_scsi_unregister and
> > e1000_unregister before.
> > 
> > This would make the purpose of the interface you are creating clearer,
> > IMHO.
> 
> I see where you're coming from, especially with the symmetry with block
> devices.

The purpose of the pci unregister function, right.

> However, the way I see it is that the VLANClientState should "own" the
> PCIDevice, not the other way around - e.g. you want to free the device,
> you should do qemu_del_vlan_client(), rather than
> pci_device_unregister(). 
> 
> What follows from that is VLANClientState::cleanup() should call
> pci_device_unregister().
> 
> If we did it the other way, then PCIDevice::unregister() should do
> qemu_del_vlan_client() and callers should never free a PCI NIC directly
> using del_vlan_client(), but instead call pci_device_unregister().
> 
> Futhermore, if we took the latter approach, we'd need a similar
> abstraction to PCIDevice for the non-PCI NICs.
> 
> As for splitting the cleanups non-device specific and device specific
> parts ... for most devices, no such separation exists. We mix all the
> state up in the structure, and it's all allocated in the one place, so
> separating out the cleanup seems a bit arbitrary.

My point of view was that you have two functions that cleanup/free the
device structure, and that is a little confusing.

But its not a big deal, compared to the present bugs which the patch
fixes.

> Following up with an updated version of the patch which uses
> PCIDevice::unregister() for unregister I/O memory and
> VLANClientState::cleanup() for cleaning everything else up.
> 
> It's not perfect, by any means ... but it's a baby step in the right
> direction IMHO.

Agree.

Re: [Qemu-devel] [PATCH 1/9] Remove stray GSO code from virtio_net

[Anthony Liguori]

Mark McLoughlin wrote:
> Obviously merged from kvm-userspace accidentally.
>
> Signed-off-by: Mark McLoughlin <markmc@redhat.com

Applied series to trunk and stable.  Thanks.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Paul Brook]

On Thursday 16 April 2009, Mark McLoughlin wrote:
> However, the way I see it is that the VLANClientState should "own" the
> PCIDevice, not the other way around - e.g. you want to free the device,
> you should do qemu_del_vlan_client(), rather than
> pci_device_unregister().

I disagree. This makes it impossible to have multiport devices.
The controlling entity should be the device, not the vlan interface.

This is related to some of the issues I've raised with the machine config 
patches. IMO it's important to consider how this kind of internat interaction 
should actually work, rather than blindly implementing whatever we currently 
expose to the user.

Paul

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Anthony Liguori]

Paul Brook wrote:
> On Thursday 16 April 2009, Mark McLoughlin wrote:
>   
>> However, the way I see it is that the VLANClientState should "own" the
>> PCIDevice, not the other way around - e.g. you want to free the device,
>> you should do qemu_del_vlan_client(), rather than
>> pci_device_unregister().
>>     
>
> I disagree. This makes it impossible to have multiport devices.
> The controlling entity should be the device, not the vlan interface.
>   

I agree.  The biggest problem with the current API (and most QEMU APIs) 
is there's no way to associate host configuration with the function's 
devices implement.  Instead, host configurations are associated with 
actual devices which implies that all devices implement one function.  
This is clearly wrong.

For example, the way this all should work is (ignore the current VLAN 
concept in QEMU, it's orthogonal to this discussion):

1) Network backend is created.  As part of the configuration of the 
backend, a unique device identifier is associated with it.
2) A PCI device is created.
  a) The PCI device creates one or more Network frontends.  Each 
frontend carries the device identifier and perhaps something like a port id.
  b) When the PCI device is destroyed, it destroys any frontends it owns.

The unique device identifier could be a device tree path, or it could be 
something more opaque that is later correlated to a device tree path.

Regards,

Anthony Liguori

> This is related to some of the issues I've raised with the machine config 
> patches. IMO it's important to consider how this kind of internat interaction 
> should actually work, rather than blindly implementing whatever we currently 
> expose to the user.
>
> Paul
>
>
>   

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Paul Brook]

> 1) Network backend is created.  As part of the configuration of the
> backend, a unique device identifier is associated with it.
> 2) A PCI device is created.
>   a) The PCI device creates one or more Network frontends.  Each
> frontend carries the device identifier and perhaps something like a port
> id. b) When the PCI device is destroyed, it destroys any frontends it owns.

This sounds about right, though I'm not entirely sure what you mean 
by "network backend". backend ~= vlan in the current implementation?

Paul

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Anthony Liguori]

Paul Brook wrote:
>> 1) Network backend is created.  As part of the configuration of the
>> backend, a unique device identifier is associated with it.
>> 2) A PCI device is created.
>>   a) The PCI device creates one or more Network frontends.  Each
>> frontend carries the device identifier and perhaps something like a port
>> id. b) When the PCI device is destroyed, it destroys any frontends it owns.
>>     
>
> This sounds about right, though I'm not entirely sure what you mean 
> by "network backend". backend ~= vlan in the current implementation?
>   

Roughly, VLANClientState would be the backend.  There are too many 
details about VLAN though in VLANClientState today beyond the name.

Ideally, the network backend would just consistent of an interface to 
submit packets, register buffers to receive packets, an interface to 
negotiate/enable features (GSO, checksum offload), and a means to get 
configuration information (MAC address).

Today, you're given a VLAN and then you create a VLANClient from the 
VLAN.  That's too much implementation detail to live in the devices IMHO.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Paul Brook]

> >> 1) Network backend is created.  As part of the configuration of the
> >> backend, a unique device identifier is associated with it.
> >> 2) A PCI device is created.
> >>   a) The PCI device creates one or more Network frontends.  Each
> >> frontend carries the device identifier and perhaps something like a port
> >> id. b) When the PCI device is destroyed, it destroys any frontends it
> >> owns.
> >
> > This sounds about right, though I'm not entirely sure what you mean
> > by "network backend". backend ~= vlan in the current implementation?
>
> Roughly, VLANClientState would be the backend.  There are too many
> details about VLAN though in VLANClientState today beyond the name.
>
> Ideally, the network backend would just consistent of an interface to
> submit packets, register buffers to receive packets, an interface to
> negotiate/enable features (GSO, checksum offload), and a means to get
> configuration information (MAC address).


Hmm, in that case I don't understand your distinction between frontend and 
backend.

> Today, you're given a VLAN and then you create a VLANClient from the
> VLAN.  That's too much implementation detail to live in the devices IMHO.

Right.

Paul

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Anthony Liguori]

Paul Brook wrote:
> Hmm, in that case I don't understand your distinction between frontend and 
> backend.
>   

In the case of networking, they don't have to be distinct because all 
you need to do is have two "front-ends" and flip the TX/RX queues.  
Although even in this case, someone has to own the MAC address so it's 
not purely symmetric.

In the general case, that isn't always true for devices.  Consider block 
devices, for instance.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Paul Brook]

On Tuesday 28 April 2009, Anthony Liguori wrote:
> Paul Brook wrote:
> > Hmm, in that case I don't understand your distinction between frontend
> > and backend.
>
> In the case of networking, they don't have to be distinct because all
> you need to do is have two "front-ends" and flip the TX/RX queues.
> Although even in this case, someone has to own the MAC address so it's
> not purely symmetric.

I'm still not understanding. Ethernet devices are fundamentally based around a 
bus architecture. "flip the TX/RX queues" only makes sense if you're talking 
about a point-point connection. For ethernet devices I see no reason to 
distinguish between "host" devices (slirp, vde, tap) and "guest" devices. 
They may be created for different reasons, but they're all doing 
fundamentally the same thing.

> In the general case, that isn't always true for devices.  Consider block
> devices, for instance.

You mean the API we expose to the devices v.s. the API we expose to the image 
file backends? Or do you mean different layers like ide/scsi v.s. internal 
block devices?

Paul

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Anthony Liguori]

Paul Brook wrote:
> On Tuesday 28 April 2009, Anthony Liguori wrote:
>   
>> Paul Brook wrote:
>>     
>>> Hmm, in that case I don't understand your distinction between frontend
>>> and backend.
>>>       
>> In the case of networking, they don't have to be distinct because all
>> you need to do is have two "front-ends" and flip the TX/RX queues.
>> Although even in this case, someone has to own the MAC address so it's
>> not purely symmetric.
>>     
>
> I'm still not understanding. Ethernet devices are fundamentally based around a 
> bus architecture. "flip the TX/RX queues" only makes sense if you're talking 
> about a point-point connection. For ethernet devices I see no reason to 
> distinguish between "host" devices (slirp, vde, tap) and "guest" devices. 
> They may be created for different reasons, but they're all doing 
> fundamentally the same thing.
>   

In the bus model, there's an implicit copy-to-the-wire operation that 
results in replication of the packet to everything else on the bus.  
 From a performance perspective, this is not at all ideal.

So really, what we're talking about is the difference between an API 
that consists of:

net->transmit_packet(data, size)
net_receive_packet(data, size) {
}

verses

net->add_transmit_packet(data, size);
datum = net->add_receive_packet(data, size);
net_notify_received_packets(datum) {
}

If the device visible API implies a copy, we lack the ability to do 
zero-copy receive.

That's not saying we shouldn't have the VLAN mechanism in QEMU but that 
should sit above a network backend that would then do the necessary copying.

And yeah, that means that we cannot have the tap implementation have an 
identical API to the devices.  This is necessary though from a 
performance perspective.

>> In the general case, that isn't always true for devices.  Consider block
>> devices, for instance.
>>     
>
> You mean the API we expose to the devices v.s. the API we expose to the image 
> file backends? Or do you mean different layers like ide/scsi v.s. internal 
> block devices?
>   

I mean the interface IDE/SCSI use to read/write data from a block device 
is not the same interface that block-qcow2 uses to provide support for 
qcow2 images.  Your argument IIUC is that block devices are not 
inherently bus oriented and this is certainly true.  However, I'm 
arguing that bus oriented APIs imply copies to the bus which is not 
something we want to bake into the interface.

So my example of block devices are irrelevant because I didn't 
understand what you originally were commenting on.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> Paul Brook wrote:
>> On Thursday 16 April 2009, Mark McLoughlin wrote:
>>   
>>> However, the way I see it is that the VLANClientState should "own" the
>>> PCIDevice, not the other way around - e.g. you want to free the device,
>>> you should do qemu_del_vlan_client(), rather than
>>> pci_device_unregister().
>>>     
>>
>> I disagree. This makes it impossible to have multiport devices.
>> The controlling entity should be the device, not the vlan interface.
>>   
>
> I agree.  The biggest problem with the current API (and most QEMU
> APIs) is there's no way to associate host configuration with the
> function's devices implement.  Instead, host configurations are
> associated with actual devices which implies that all devices
> implement one function.  This is clearly wrong.
>
> For example, the way this all should work is (ignore the current VLAN
> concept in QEMU, it's orthogonal to this discussion):
>
> 1) Network backend is created.  As part of the configuration of the
> backend, a unique device identifier is associated with it.
> 2) A PCI device is created.
>  a) The PCI device creates one or more Network frontends.  Each
> frontend carries the device identifier and perhaps something like a
> port id.
>  b) When the PCI device is destroyed, it destroys any frontends it owns.
>
> The unique device identifier could be a device tree path, or it could
> be something more opaque that is later correlated to a device tree
> path.
>
> Regards,
>
> Anthony Liguori

The machine configuration patch is designed for such a separation.  A
target device can be connected to host artifacts.  The connection is a
tree node UID.  Currently the node address, but it could be anything; we
could use small integer handles or textual labels.

The initial implementation of a device is a simple wrapper around
existing device interfaces.  This is intentional.  It lets us convert to
the new device interface device by device.  Such a conversion involves
disentangling host and target part, putting the former in suitable host
artifacts, leaving the latter in the target device.

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Paul Brook]

> > I'm still not understanding. Ethernet devices are fundamentally based
> > around a bus architecture. "flip the TX/RX queues" only makes sense if
> > you're talking about a point-point connection. For ethernet devices I see
> > no reason to distinguish between "host" devices (slirp, vde, tap) and
> > "guest" devices. They may be created for different reasons, but they're
> > all doing fundamentally the same thing.
>
> In the bus model, there's an implicit copy-to-the-wire operation that
> results in replication of the packet to everything else on the bus.
>  From a performance perspective, this is not at all ideal.

I'm not convinced by this argument. I don't see why a one-many api requires a 
copy.

Paul

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Avi Kivity]

Paul Brook wrote:
>>> I'm still not understanding. Ethernet devices are fundamentally based
>>> around a bus architecture. "flip the TX/RX queues" only makes sense if
>>> you're talking about a point-point connection. For ethernet devices I see
>>> no reason to distinguish between "host" devices (slirp, vde, tap) and
>>> "guest" devices. They may be created for different reasons, but they're
>>> all doing fundamentally the same thing.
>>>       
>> In the bus model, there's an implicit copy-to-the-wire operation that
>> results in replication of the packet to everything else on the bus.
>>  From a performance perspective, this is not at all ideal.
>>     
>
> I'm not convinced by this argument. I don't see why a one-many api requires a 
> copy.
>   

To achieve copyless receive (from guest perspective), you need to give 
the receive buffer to the transmitting device (tap).  If there are more 
than one transmitting devices, you have a conflict.

Sure, you can short-ciruit the case where you have a pair of devices, 
but then you need to revoke posted receive descriptors.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Paul Brook]

> Sure, you can short-ciruit the case where you have a pair of devices,
> but then you need to revoke posted receive descriptors.

Isn't the same true with a point-point API? When you add a third device to the 
vlan you're going to have to break the tap-guest link, and insert a proxy in 
the middle.

Paul

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Avi Kivity]

Paul Brook wrote:
>> Sure, you can short-ciruit the case where you have a pair of devices,
>> but then you need to revoke posted receive descriptors.
>>     
>
> Isn't the same true with a point-point API? When you add a third device to the 
> vlan you're going to have to break the tap-guest link, and insert a proxy in 
> the middle.
>   

I thought with a point-to-point API, you would not actually be inserting 
any third device.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Paul Brook]

On Thursday 30 April 2009, Avi Kivity wrote:
> Paul Brook wrote:
> >> Sure, you can short-ciruit the case where you have a pair of devices,
> >> but then you need to revoke posted receive descriptors.
> >
> > Isn't the same true with a point-point API? When you add a third device
> > to the vlan you're going to have to break the tap-guest link, and insert
> > a proxy in the middle.
>
> I thought with a point-to-point API, you would not actually be inserting
> any third device.

That's a fair chunk of functionality you just lost then.

Paul

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Anthony Liguori]

Paul Brook wrote:
>> Sure, you can short-ciruit the case where you have a pair of devices,
>> but then you need to revoke posted receive descriptors.
>>     
>
> Isn't the same true with a point-point API? When you add a third device to the 
> vlan you're going to have to break the tap-guest link, and insert a proxy in 
> the middle.
>   

The difference is that if you assume vlan, then you have to handle 
renegotiation of features which is not actually possible.  With 
virtio-net, once you enable guest GSO, you cannot disable it.  You would 
have to implement GSO emulation within QEMU if you wanted to disable it 
on the tap device.

If you don't assume vlan, then you can choose the least amount of 
features when in a vlan.  Then you never have to deal with this problem.

And yes, that means that if you wanted to support the existing syntax, 
you would need a flag like:

-net tap,i-will-never-add-more-devices-to-this-vlan=on -net nic,model=virtio

Although we should just add a new syntax.  Honestly, vlans have to be 
the least useful feature in QEMU.  Why in the world would anyone ever 
put more than one nic on a vlan?  I understand why the bus architecture 
is appealing from a symmetry perspective but it's not at all useful from 
a user perspective.

> Paul
>   


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Avi Kivity]

Paul Brook wrote:
> On Thursday 30 April 2009, Avi Kivity wrote:
>   
>> Paul Brook wrote:
>>     
>>>> Sure, you can short-ciruit the case where you have a pair of devices,
>>>> but then you need to revoke posted receive descriptors.
>>>>         
>>> Isn't the same true with a point-point API? When you add a third device
>>> to the vlan you're going to have to break the tap-guest link, and insert
>>> a proxy in the middle.
>>>       
>> I thought with a point-to-point API, you would not actually be inserting
>> any third device.
>>     
>
> That's a fair chunk of functionality you just lost then.
>   

True, but is it actually useful?

Short-circuiting two guest devices probably isn't.  Adding host devices 
(like recording packets) might be.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Anthony Liguori]

Paul Brook wrote:
> On Thursday 30 April 2009, Avi Kivity wrote:
>   
>> Paul Brook wrote:
>>     
>>>> Sure, you can short-ciruit the case where you have a pair of devices,
>>>> but then you need to revoke posted receive descriptors.
>>>>         
>>> Isn't the same true with a point-point API? When you add a third device
>>> to the vlan you're going to have to break the tap-guest link, and insert
>>> a proxy in the middle.
>>>       
>> I thought with a point-to-point API, you would not actually be inserting
>> any third device.
>>     
>
> That's a fair chunk of functionality you just lost then.
>   

That no one ever actually uses and that very often confuses people.

Have I mentioned lately that I don't like qemu vlans? :-)

> Paul
>   


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Anthony Liguori]

Avi Kivity wrote:
> True, but is it actually useful?
>
> Short-circuiting two guest devices probably isn't.  Adding host 
> devices (like recording packets) might be.

But you can do this by just inserting yourself between the 
frontend/backend.  You can do this dynamically as long as you don't 
require feature negotiation.  Dynamic negotiation is prohibitively 
difficult and one of the primary reasons we haven't been able to merge 
vnet support for tap yet.

The bus architecture is only useful if you want to generate and receive 
traffic on a vlan, not if you just want to record traffic.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Anthony Liguori]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> True, but is it actually useful?
>>
>> Short-circuiting two guest devices probably isn't.  Adding host 
>> devices (like recording packets) might be.
>
> But you can do this by just inserting yourself between the 
> frontend/backend.  You can do this dynamically as long as you don't 
> require feature negotiation.  Dynamic negotiation is prohibitively 
> difficult and one of the primary reasons we haven't been able to merge 
> vnet support for tap yet.

Paul:  in KVM, tap devices support optional GSO/checksum offload 
support.  This is enabled for the tap device depending on whether you're 
using virtio-net and your host kernel supports it.

In KVM, having > 1 guest device on a vlan is broken when a tap device in 
on a vlan.  This is by design because the dynamic feature negotiation 
isn't always possible.  The other option would be to disallow additional 
VLAN that can't support the same features or do full GSO emulation 
within QEMU.

Neither of these are very good options.  No one has complained yet to 
the best of my knowledge that you can't have more than one guest device 
on a vlan in KVM.  This is simply because no one ever does it :-)

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Blue Swirl]

On 4/30/09, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Paul Brook wrote:
>
> >
> > > Sure, you can short-ciruit the case where you have a pair of devices,
> > > but then you need to revoke posted receive descriptors.
> > >
> > >
> >
> > Isn't the same true with a point-point API? When you add a third device to
> the vlan you're going to have to break the tap-guest link, and insert a
> proxy in the middle.
> >
> >
>
>  The difference is that if you assume vlan, then you have to handle
> renegotiation of features which is not actually possible.  With virtio-net,
> once you enable guest GSO, you cannot disable it.  You would have to
> implement GSO emulation within QEMU if you wanted to disable it on the tap
> device.
>
>  If you don't assume vlan, then you can choose the least amount of features
> when in a vlan.  Then you never have to deal with this problem.
>
>  And yes, that means that if you wanted to support the existing syntax, you
> would need a flag like:
>
>  -net tap,i-will-never-add-more-devices-to-this-vlan=on
> -net nic,model=virtio
>

-net pointopoint,tap,nic,model=virtio ?

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Paul Brook]

> Neither of these are very good options.  

Agreed. Bolting point-point requirements onto the vlan API seem like a really 
bad idea.

> No one has complained yet to 
> the best of my knowledge that you can't have more than one guest device
> on a vlan in KVM.  This is simply because no one ever does it :-)

I'm not so sure about that. I don't believe kvm is representative of qemu as a 
whole. My guess is your typical kvm user cares more about netwroking 
performance then they do about (for example) requiring root privileges to 
instantiate virtual machines.

That said, I guess vlan functionality can be punted to something like vde.

Paul

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Anthony Liguori]

Blue Swirl wrote:
> On 4/30/09, Anthony Liguori <aliguori@us.ibm.com> wrote:
>   
> -net pointopoint,tap,nic,model=virtio ?
>   

Or perhaps:

-net tap,vlan=off,id=1234 -net nic,model=virtio,vlan=off,id=1234

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 543 bytes --]

Anthony Liguori wrote:
> Blue Swirl wrote:
>> On 4/30/09, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>   -net pointopoint,tap,nic,model=virtio ?
>>   
> 
> Or perhaps:
> 
> -net tap,vlan=off,id=1234 -net nic,model=virtio,vlan=off,id=1234
> 

That would only allow one such pair per VM.

Why not keeping all the existing infrastructure, just locking a vlan
against becoming more than a point-to-point link once some conflicting
optimization was applied? That should be easy to implement and to
explain to the user.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Avi Kivity]

Jan Kiszka wrote:
> Anthony Liguori wrote:
>   
>> Blue Swirl wrote:
>>     
>>> On 4/30/09, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>>   -net pointopoint,tap,nic,model=virtio ?
>>>   
>>>       
>> Or perhaps:
>>
>> -net tap,vlan=off,id=1234 -net nic,model=virtio,vlan=off,id=1234
>>
>>     
>
> That would only allow one such pair per VM.
>   

I'm guessing that's what id= is for.

> Why not keeping all the existing infrastructure, just locking a vlan
> against becoming more than a point-to-point link once some conflicting
> optimization was applied? That should be easy to implement and to
> explain to the user.
>   

I still haven't seen a real life usecase for >2 terminals on a vlan.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Avi Kivity]

Paul Brook wrote:
>> No one has complained yet to 
>> the best of my knowledge that you can't have more than one guest device
>> on a vlan in KVM.  This is simply because no one ever does it :-)
>>     
>
> I'm not so sure about that. I don't believe kvm is representative of qemu as a 
> whole. 

I agree with this, but...

> My guess is your typical kvm user cares more about netwroking 
> performance then they do about (for example) requiring root privileges to 
> instantiate virtual machines.
>   

In fact kvm discourages root privileges; that's why I like fd= options, 
and want them supported on the monitor (via SCM_RIGHTS).

> That said, I guess vlan functionality can be punted to something like vde.
>   

Yes please.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Anthony Liguori]

Jan Kiszka wrote:
> That would only allow one such pair per VM.
>   

id basically becomes another type of vlan id.  To have multiple nics, 
you do:

-net tap,vlan=off,id=1234 -net nic,model=virtio,vlan=off,id=1234
-net tap,vlan=off,id=4321 -net nic,model=virtio,vlan=off,id=4321

And this goes back to the notion of having all device 
front-ends/back-ends have some sort of identifier to associate one to 
the other.

> Why not keeping all the existing infrastructure, just locking a vlan
> against becoming more than a point-to-point link once some conflicting
> optimization was applied? That should be easy to implement and to
> explain to the user.
>   

I think you're suggesting the same thing as me, except you are saying 
make vlan=off implicit, and use vlan=XXX instead of id=XXX.

We can still make vlan=off implicit, and default id=0, so that -net tap 
net nic,model=virtio does the right thing.  However, if a user 
explicitly says -net tap,vlan=1 -net nic,model=virtio,vlan=1, it behaves 
like it used to.

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Blue Swirl]

On 4/30/09, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Jan Kiszka wrote:
>
> > That would only allow one such pair per VM.
> >
> >
>
>  id basically becomes another type of vlan id.  To have multiple nics, you
> do:
>
>  -net tap,vlan=off,id=1234 -net
> nic,model=virtio,vlan=off,id=1234
>  -net tap,vlan=off,id=4321 -net
> nic,model=virtio,vlan=off,id=4321

I think "off" and "id" are not descriptive enough, how about:
 -net tap,vlan=pointopoint,ptop_id=4321 -net
nic,model=virtio,vlan=pointopoint,ptop_id=4321

>  And this goes back to the notion of having all device front-ends/back-ends
> have some sort of identifier to associate one to the other.
>
>
> > Why not keeping all the existing infrastructure, just locking a vlan
> > against becoming more than a point-to-point link once some conflicting
> > optimization was applied? That should be easy to implement and to
> > explain to the user.
> >
> >
>
>  I think you're suggesting the same thing as me, except you are saying make
> vlan=off implicit, and use vlan=XXX instead of id=XXX.
>
>  We can still make vlan=off implicit, and default id=0, so that -net tap net
> nic,model=virtio does the right thing.  However, if a user explicitly says
> -net tap,vlan=1 -net nic,model=virtio,vlan=1, it behaves like it used to.

Nice, though if there are two vlans, one specified without explicit ID
and the other with ID=1, the performance will be different.

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1439 bytes --]

Anthony Liguori wrote:
> Jan Kiszka wrote:
>> That would only allow one such pair per VM.
>>   
> 
> id basically becomes another type of vlan id.  To have multiple nics,
> you do:
> 
> -net tap,vlan=off,id=1234 -net nic,model=virtio,vlan=off,id=1234
> -net tap,vlan=off,id=4321 -net nic,model=virtio,vlan=off,id=4321
> 
> And this goes back to the notion of having all device
> front-ends/back-ends have some sort of identifier to associate one to
> the other.
> 
>> Why not keeping all the existing infrastructure, just locking a vlan
>> against becoming more than a point-to-point link once some conflicting
>> optimization was applied? That should be easy to implement and to
>> explain to the user.
>>   
> 
> I think you're suggesting the same thing as me, except you are saying
> make vlan=off implicit, and use vlan=XXX instead of id=XXX.

Look like. :)

> 
> We can still make vlan=off implicit, and default id=0, so that -net tap
> net nic,model=virtio does the right thing.  However, if a user
> explicitly says -net tap,vlan=1 -net nic,model=virtio,vlan=1, it behaves
> like it used to.

That's my point. And if he tries "-net tap -net nic,model=virtio -net
nic", qemu will fall back to bus-like vlan. And if he tries do add the
third nic during runtime, there will be a descriptive error message.
That should be most elegant while not blocking optimizations (of the
common case).

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

Avi Kivity wrote:
> Jan Kiszka wrote:
>> Why not keeping all the existing infrastructure, just locking a vlan
>> against becoming more than a point-to-point link once some conflicting
>> optimization was applied? That should be easy to implement and to
>> explain to the user.
>>   
> 
> I still haven't seen a real life usecase for >2 terminals on a vlan.
> 

Inter-VM linking e.g:

qemu -net user -net nic -net socket,listen=...
qemu -net socket,connect=...

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Avi Kivity]

Jan Kiszka wrote:
> Avi Kivity wrote:
>   
>> Jan Kiszka wrote:
>>     
>>> Why not keeping all the existing infrastructure, just locking a vlan
>>> against becoming more than a point-to-point link once some conflicting
>>> optimization was applied? That should be easy to implement and to
>>> explain to the user.
>>>   
>>>       
>> I still haven't seen a real life usecase for >2 terminals on a vlan.
>>
>>     
>
> Inter-VM linking e.g:
>
> qemu -net user -net nic -net socket,listen=...
> qemu -net socket,connect=...
>
>   

Hmm, okay.  But it's a bit sad to carry all that complexity for this.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]

Blue Swirl wrote:
> On 4/30/09, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Jan Kiszka wrote:
>>> Why not keeping all the existing infrastructure, just locking a vlan
>>> against becoming more than a point-to-point link once some conflicting
>>> optimization was applied? That should be easy to implement and to
>>> explain to the user.
>>>
>>>
>>  I think you're suggesting the same thing as me, except you are saying make
>> vlan=off implicit, and use vlan=XXX instead of id=XXX.
>>
>>  We can still make vlan=off implicit, and default id=0, so that -net tap net
>> nic,model=virtio does the right thing.  However, if a user explicitly says
>> -net tap,vlan=1 -net nic,model=virtio,vlan=1, it behaves like it used to.
> 
> Nice, though if there are two vlans, one specified without explicit ID
> and the other with ID=1, the performance will be different.

Uh, the second case should rather behave like the first. Only if the
number vlan clients grows beyond 2, point-to-point optimizations should
be disabled (unless already enabled, then adding clients should be denied).

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Anthony Liguori]

Avi Kivity wrote:
> Hmm, okay.  But it's a bit sad to carry all that complexity for this.

It's a feature people do use so I'd prefer not to drop it if possible.

Regards,

Anthony Liguori



-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Anthony Liguori]

Blue Swirl wrote:
> On 4/30/09, Anthony Liguori <aliguori@us.ibm.com> wrote:
>   
>> Jan Kiszka wrote:
>>
>>     
>>> That would only allow one such pair per VM.
>>>
>>>
>>>       
>>  id basically becomes another type of vlan id.  To have multiple nics, you
>> do:
>>
>>  -net tap,vlan=off,id=1234 -net
>> nic,model=virtio,vlan=off,id=1234
>>  -net tap,vlan=off,id=4321 -net
>> nic,model=virtio,vlan=off,id=4321
>>     
>
> I think "off" and "id" are not descriptive enough, how about:
>  -net tap,vlan=pointopoint,ptop_id=4321 -net
> nic,model=virtio,vlan=pointopoint,ptop_id=4321
>   

or vlan=none?  p2p doesn't make very much sense to me personally.

I agree "id" may be too generic.  Maybe devid or device_id?

>>  I think you're suggesting the same thing as me, except you are saying make
>> vlan=off implicit, and use vlan=XXX instead of id=XXX.
>>
>>  We can still make vlan=off implicit, and default id=0, so that -net tap net
>> nic,model=virtio does the right thing.  However, if a user explicitly says
>> -net tap,vlan=1 -net nic,model=virtio,vlan=1, it behaves like it used to.
>>     
>
> Nice, though if there are two vlans, one specified without explicit ID
> and the other with ID=1, the performance will be different.
>   

If a user mixes vlans and p2p links, then yeah, performance is going to 
be different.  I'd like to eventually make vlan=none the default to be 
perfectly honest.  I don't think many people depend on implicit vlan=0 
so I don't think we'll really break anyone.

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Blue Swirl]

On 4/30/09, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Blue Swirl wrote:
>
> > On 4/30/09, Anthony Liguori <aliguori@us.ibm.com> wrote:
> >
> >
> > > Jan Kiszka wrote:
> > >
> > >
> > >
> > > > That would only allow one such pair per VM.
> > > >
> > > >
> > > >
> > > >
> > >  id basically becomes another type of vlan id.  To have multiple nics,
> you
> > > do:
> > >
> > >  -net tap,vlan=off,id=1234 -net
> > > nic,model=virtio,vlan=off,id=1234
> > >  -net tap,vlan=off,id=4321 -net
> > > nic,model=virtio,vlan=off,id=4321
> > >
> > >
> >
> > I think "off" and "id" are not descriptive enough, how about:
> >  -net tap,vlan=pointopoint,ptop_id=4321 -net
> > nic,model=virtio,vlan=pointopoint,ptop_id=4321
> >
> >
>
>  or vlan=none?  p2p doesn't make very much sense to me personally.

That's good too.

>  I agree "id" may be too generic.  Maybe devid or device_id?

That may be confused with for example PCI ID. pair_id?

> > >  I think you're suggesting the same thing as me, except you are saying
> make
> > > vlan=off implicit, and use vlan=XXX instead of id=XXX.
> > >
> > >  We can still make vlan=off implicit, and default id=0, so that -net tap
> net
> > > nic,model=virtio does the right thing.  However, if a user explicitly
> says
> > > -net tap,vlan=1 -net nic,model=virtio,vlan=1, it behaves like it used
> to.
> > >
> > >
> >
> > Nice, though if there are two vlans, one specified without explicit ID
> > and the other with ID=1, the performance will be different.
> >
> >
>
>  If a user mixes vlans and p2p links, then yeah, performance is going to be
> different.  I'd like to eventually make vlan=none the default to be
> perfectly honest.  I don't think many people depend on implicit vlan=0 so I
> don't think we'll really break anyone.

No, and also different performance won't matter.