[Qemu-devel] [PATCH 0/5] virtio-serial: Fixes and cleanups

[Qemu-devel] [PATCH 0/5] virtio-serial: Fixes and cleanups

[Markus Armbruster]

Feel free to drop PATCH 3/3 if you believe we need to keep the useless
property is_console for backward compatibility.

Markus Armbruster (5):
  virtio-serial: Plug memory leak on qdev exit()
  virtio-serial: Clean up virtconsole detection
  virtio-serial: Drop useless property is_console
  virtio-serial: Drop redundant VirtIOSerialPort member info
  virtio-console: Simplify init callbacks

 hw/virtio-console.c    |   47 +++++++++++---------------------
 hw/virtio-serial-bus.c |   71 ++++++++++++++++++++++++-----------------------
 hw/virtio-serial.h     |    8 +++---
 3 files changed, 56 insertions(+), 70 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 1/5] virtio-serial: Plug memory leak on qdev exit()

[Markus Armbruster]

virtio_serial_init() allocates the VirtIOSerialBus dynamically, but
virtio_serial_exit() doesn't free it.

Fix by getting rid of the allocation.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio-serial-bus.c |   27 +++++++++------------------
 1 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index f10d48f..adbcf6a 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -39,7 +39,7 @@ struct VirtIOSerial {
     /* Arrays of ivqs and ovqs: one per port */
     VirtQueue **ivqs, **ovqs;
 
-    VirtIOSerialBus *bus;
+    VirtIOSerialBus bus;
 
     DeviceState *qdev;
 
@@ -325,7 +325,7 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
     case VIRTIO_CONSOLE_DEVICE_READY:
         if (!cpkt.value) {
             error_report("virtio-serial-bus: Guest failure in adding device %s\n",
-                         vser->bus->qbus.name);
+                         vser->bus.qbus.name);
             break;
         }
         /*
@@ -340,7 +340,7 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
     case VIRTIO_CONSOLE_PORT_READY:
         if (!cpkt.value) {
             error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n",
-                         port->id, vser->bus->qbus.name);
+                         port->id, vser->bus.qbus.name);
             break;
         }
         /*
@@ -467,7 +467,7 @@ static uint32_t get_features(VirtIODevice *vdev, uint32_t features)
 
     vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
 
-    if (vser->bus->max_nr_ports > 1) {
+    if (vser->bus.max_nr_ports > 1) {
         features |= (1 << VIRTIO_CONSOLE_F_MULTIPORT);
     }
     return features;
@@ -644,16 +644,6 @@ static struct BusInfo virtser_bus_info = {
     .print_dev = virtser_bus_dev_print,
 };
 
-static VirtIOSerialBus *virtser_bus_new(DeviceState *dev)
-{
-    VirtIOSerialBus *bus;
-
-    bus = FROM_QBUS(VirtIOSerialBus, qbus_create(&virtser_bus_info, dev, NULL));
-    bus->qbus.allow_hotplug = 1;
-
-    return bus;
-}
-
 static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
 {
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, qdev);
@@ -835,11 +825,12 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *conf)
     vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
 
     /* Spawn a new virtio-serial bus on which the ports will ride as devices */
-    vser->bus = virtser_bus_new(dev);
-    vser->bus->vser = vser;
+    qbus_create_inplace(&vser->bus.qbus, &virtser_bus_info, dev, NULL);
+    vser->bus.qbus.allow_hotplug = 1;
+    vser->bus.vser = vser;
     QTAILQ_INIT(&vser->ports);
 
-    vser->bus->max_nr_ports = conf->max_virtserial_ports;
+    vser->bus.max_nr_ports = conf->max_virtserial_ports;
     vser->ivqs = qemu_malloc(conf->max_virtserial_ports * sizeof(VirtQueue *));
     vser->ovqs = qemu_malloc(conf->max_virtserial_ports * sizeof(VirtQueue *));
 
@@ -859,7 +850,7 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *conf)
     /* control queue: guest to host */
     vser->c_ovq = virtio_add_queue(vdev, 32, control_out);
 
-    for (i = 1; i < vser->bus->max_nr_ports; i++) {
+    for (i = 1; i < vser->bus.max_nr_ports; i++) {
         /* Add a per-port queue for host to guest transfers */
         vser->ivqs[i] = virtio_add_queue(vdev, 128, handle_input);
         /* Add a per-per queue for guest to host transfers */
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/5] virtio-serial: Clean up virtconsole detection

[Markus Armbruster]

virtio-serial-bus needs to treat "virtconsole" devices specially.  It
uses VirtIOSerialPort member is_console to recognize them.  It gets
its value via property initialization.  Cute hack, except it lets
users mess with it: "-device virtconsole,is_console=0" isn't plugged
into port 0 as it should.

Move the flag to VirtIOSerialPortInfo.  Keep the property for backward
compatibility; its value has no effect.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio-console.c    |    5 +++--
 hw/virtio-serial-bus.c |    4 ++--
 hw/virtio-serial.h     |    8 ++++++--
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index de539c4..50b85f8 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -91,7 +91,7 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    port->is_console = true;
+    port->is_console_dummy = true;
     return generic_port_init(vcon, port);
 }
 
@@ -113,10 +113,11 @@ static int virtconsole_exitfn(VirtIOSerialPort *port)
 static VirtIOSerialPortInfo virtconsole_info = {
     .qdev.name     = "virtconsole",
     .qdev.size     = sizeof(VirtConsole),
+    .is_console    = true,
     .init          = virtconsole_initfn,
     .exit          = virtconsole_exitfn,
     .qdev.props = (Property[]) {
-        DEFINE_PROP_UINT8("is_console", VirtConsole, port.is_console, 1),
+        DEFINE_PROP_UINT8("is_console", VirtConsole, port.is_console_dummy, 1),
         DEFINE_PROP_UINT32("nr", VirtConsole, port.id, VIRTIO_CONSOLE_BAD_ID),
         DEFINE_PROP_CHR("chardev", VirtConsole, chr),
         DEFINE_PROP_STRING("name", VirtConsole, port.name),
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index adbcf6a..d0a4ade 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -350,7 +350,7 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
          * this port is a console port so that the guest can hook it
          * up to hvc.
          */
-        if (port->is_console) {
+        if (port->info->is_console) {
             send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1);
         }
 
@@ -722,7 +722,7 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
      * location 0. This is done for backward compatibility (old
      * kernel, new qemu).
      */
-    plugging_port0 = port->is_console && !find_port_by_id(port->vser, 0);
+    plugging_port0 = info->is_console && !find_port_by_id(port->vser, 0);
 
     if (find_port_by_id(port->vser, port->id)) {
         error_report("virtio-serial-bus: A port already exists at id %u\n",
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 5eb948e..c0d1717 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -119,8 +119,8 @@ struct VirtIOSerialPort {
     uint32_t iov_idx;
     uint64_t iov_offset;
 
-    /* Identify if this is a port that binds with hvc in the guest */
-    uint8_t is_console;
+    /* For property backward compatibility, not used otherwise */
+    uint8_t is_console_dummy;
 
     /* Is the corresponding guest device open? */
     bool guest_connected;
@@ -132,6 +132,10 @@ struct VirtIOSerialPort {
 
 struct VirtIOSerialPortInfo {
     DeviceInfo qdev;
+
+    /* Is this a device that binds with hvc in the guest? */
+    bool is_console;
+
     /*
      * The per-port (or per-app) init function that's called when a
      * new device is found on the bus.
-- 
1.7.2.3

[Qemu-devel] [PATCH 3/5] virtio-serial: Drop useless property is_console

[Markus Armbruster]

All you could ever achieve with it is break stuff, so removing it
should be safe.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio-console.c |    2 --
 hw/virtio-serial.h  |    3 ---
 2 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 50b85f8..180ac0a 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -91,7 +91,6 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    port->is_console_dummy = true;
     return generic_port_init(vcon, port);
 }
 
@@ -117,7 +116,6 @@ static VirtIOSerialPortInfo virtconsole_info = {
     .init          = virtconsole_initfn,
     .exit          = virtconsole_exitfn,
     .qdev.props = (Property[]) {
-        DEFINE_PROP_UINT8("is_console", VirtConsole, port.is_console_dummy, 1),
         DEFINE_PROP_UINT32("nr", VirtConsole, port.id, VIRTIO_CONSOLE_BAD_ID),
         DEFINE_PROP_CHR("chardev", VirtConsole, chr),
         DEFINE_PROP_STRING("name", VirtConsole, port.name),
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index c0d1717..acf049d 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -119,9 +119,6 @@ struct VirtIOSerialPort {
     uint32_t iov_idx;
     uint64_t iov_offset;
 
-    /* For property backward compatibility, not used otherwise */
-    uint8_t is_console_dummy;
-
     /* Is the corresponding guest device open? */
     bool guest_connected;
     /* Is this device open for IO on the host? */
-- 
1.7.2.3

[Qemu-devel] [PATCH 4/5] virtio-serial: Drop redundant VirtIOSerialPort member info

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio-console.c    |    9 ++++++---
 hw/virtio-serial-bus.c |   42 ++++++++++++++++++++++++++----------------
 hw/virtio-serial.h     |    1 -
 3 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 180ac0a..713f6ef 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -76,12 +76,15 @@ static void chr_event(void *opaque, int event)
 
 static int generic_port_init(VirtConsole *vcon, VirtIOSerialPort *port)
 {
+    VirtIOSerialPortInfo *info = DO_UPCAST(VirtIOSerialPortInfo, qdev,
+                                           vcon->port.dev.info);
+
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
-        vcon->port.info->have_data = flush_buf;
-        vcon->port.info->guest_open = guest_open;
-        vcon->port.info->guest_close = guest_close;
+        info->have_data = flush_buf;
+        info->guest_open = guest_open;
+        info->guest_close = guest_close;
     }
     return 0;
 }
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index d0a4ade..e85315d 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -129,9 +129,13 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
 static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
                                  VirtIODevice *vdev)
 {
+    VirtIOSerialPortInfo *info;
+
     assert(port);
     assert(virtio_queue_ready(vq));
 
+    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
+
     while (!port->throttled) {
         unsigned int i;
 
@@ -149,10 +153,10 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
             ssize_t ret;
 
             buf_size = port->elem.out_sg[i].iov_len - port->iov_offset;
-            ret = port->info->have_data(port,
-                                        port->elem.out_sg[i].iov_base
-                                          + port->iov_offset,
-                                        buf_size);
+            ret = info->have_data(port,
+                                  port->elem.out_sg[i].iov_base
+                                  + port->iov_offset,
+                                  buf_size);
             if (ret < 0 && ret != -EAGAIN) {
                 /* We don't handle any other type of errors here */
                 abort();
@@ -303,6 +307,7 @@ void virtio_serial_throttle_port(VirtIOSerialPort *port, bool throttle)
 static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
 {
     struct VirtIOSerialPort *port;
+    struct VirtIOSerialPortInfo *info;
     struct virtio_console_control cpkt, *gcpkt;
     uint8_t *buffer;
     size_t buffer_len;
@@ -321,6 +326,8 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
     if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
         return;
 
+    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
+
     switch(cpkt.event) {
     case VIRTIO_CONSOLE_DEVICE_READY:
         if (!cpkt.value) {
@@ -350,7 +357,7 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
          * this port is a console port so that the guest can hook it
          * up to hvc.
          */
-        if (port->info->is_console) {
+        if (info->is_console) {
             send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1);
         }
 
@@ -379,21 +386,21 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
          * initialised. If some app is interested in knowing about
          * this event, let it know.
          */
-        if (port->info->guest_ready) {
-            port->info->guest_ready(port);
+        if (info->guest_ready) {
+            info->guest_ready(port);
         }
         break;
 
     case VIRTIO_CONSOLE_PORT_OPEN:
         port->guest_connected = cpkt.value;
-        if (cpkt.value && port->info->guest_open) {
+        if (cpkt.value && info->guest_open) {
             /* Send the guest opened notification if an app is interested */
-            port->info->guest_open(port);
+            info->guest_open(port);
         }
 
-        if (!cpkt.value && port->info->guest_close) {
+        if (!cpkt.value && info->guest_close) {
             /* Send the guest closed notification if an app is interested */
-            port->info->guest_close(port);
+            info->guest_close(port);
         }
         break;
     }
@@ -442,11 +449,13 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOSerial *vser;
     VirtIOSerialPort *port;
+    VirtIOSerialPortInfo *info;
 
     vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
     port = find_port_by_vq(vser, vq);
+    info = port ? DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info) : NULL;
 
-    if (!port || !port->host_connected || !port->info->have_data) {
+    if (!port || !port->host_connected || !info->have_data) {
         discard_vq_data(vq, vdev);
         return;
     }
@@ -749,7 +758,6 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
         return -1;
     }
 
-    port->info = info;
     ret = info->init(port);
     if (ret) {
         return ret;
@@ -780,15 +788,17 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
 static int virtser_port_qdev_exit(DeviceState *qdev)
 {
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, qdev);
+    VirtIOSerialPortInfo *info = DO_UPCAST(VirtIOSerialPortInfo, qdev,
+                                           port->dev.info);
     VirtIOSerial *vser = port->vser;
 
     remove_port(port->vser, port->id);
 
     QTAILQ_REMOVE(&vser->ports, port, next);
 
-    if (port->info->exit)
-        port->info->exit(port);
-
+    if (info->exit) {
+        info->exit(port);
+    }
     return 0;
 }
 
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index acf049d..f267404 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -75,7 +75,6 @@ typedef struct VirtIOSerialPortInfo VirtIOSerialPortInfo;
  */
 struct VirtIOSerialPort {
     DeviceState dev;
-    VirtIOSerialPortInfo *info;
 
     QTAILQ_ENTRY(VirtIOSerialPort) next;
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 5/5] virtio-console: Simplify init callbacks

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio-console.c |   35 +++++++++--------------------------
 1 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 713f6ef..b076331 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -74,11 +74,17 @@ static void chr_event(void *opaque, int event)
     }
 }
 
-static int generic_port_init(VirtConsole *vcon, VirtIOSerialPort *port)
+static int virtconsole_initfn(VirtIOSerialPort *port)
 {
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
     VirtIOSerialPortInfo *info = DO_UPCAST(VirtIOSerialPortInfo, qdev,
                                            vcon->port.dev.info);
 
+    if (port->id == 0 && !info->is_console) {
+        error_report("Port number 0 on virtio-serial devices reserved for virtconsole devices for backward compatibility.");
+        return -1;
+    }
+
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
@@ -86,15 +92,8 @@ static int generic_port_init(VirtConsole *vcon, VirtIOSerialPort *port)
         info->guest_open = guest_open;
         info->guest_close = guest_close;
     }
-    return 0;
-}
-
-/* Virtio Console Ports */
-static int virtconsole_initfn(VirtIOSerialPort *port)
-{
-    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    return generic_port_init(vcon, port);
+    return 0;
 }
 
 static int virtconsole_exitfn(VirtIOSerialPort *port)
@@ -132,26 +131,10 @@ static void virtconsole_register(void)
 }
 device_init(virtconsole_register)
 
-/* Generic Virtio Serial Ports */
-static int virtserialport_initfn(VirtIOSerialPort *port)
-{
-    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
-
-    if (port->id == 0) {
-        /*
-         * Disallow a generic port at id 0, that's reserved for
-         * console ports.
-         */
-        error_report("Port number 0 on virtio-serial devices reserved for virtconsole devices for backward compatibility.");
-        return -1;
-    }
-    return generic_port_init(vcon, port);
-}
-
 static VirtIOSerialPortInfo virtserialport_info = {
     .qdev.name     = "virtserialport",
     .qdev.size     = sizeof(VirtConsole),
-    .init          = virtserialport_initfn,
+    .init          = virtconsole_initfn,
     .exit          = virtconsole_exitfn,
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT32("nr", VirtConsole, port.id, VIRTIO_CONSOLE_BAD_ID),
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 0/5] virtio-serial: Fixes and cleanups

[Amit Shah]

On (Wed) 25 May 2011 [14:21:09], Markus Armbruster wrote:
> Feel free to drop PATCH 3/3 if you believe we need to keep the useless
> property is_console for backward compatibility.

I don't think anyone uses it; it's better gone.

Thanks for the patches, applied to my tree.  Will send a pull request
after running some tests.

		Amit