virtio-serial: Prevent addition of ports with same name

PATCH In review Amit Shah 3 participants 14 messages Jul 16, 2014 – Aug 19, 2014

lore

Share & tools

lore

Amit Shah

author

2014-07-16 11:32 UTC #0 lore raw

cover virtio-serial: Prevent addition of ports with same name

Hi,

The 2nd patch prevents adding ports to the system with conflicts in the 'name' parameter. This is done by going through all the ports in the system, including ports on other virtio-serial devices.

The first patch prepares for this by creating a linked list of all available virtio-serial devices, so they can be walked over to check their ports' names.

Please review.

Amit Shah (2):
virtio-serial: create a linked list of all active devices
virtio-serial: search for duplicate port names before adding new ports

 hw/char/virtio-serial-bus.c       | 32 ++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-serial.h |  2 ++
 2 files changed, 34 insertions(+)

Signature · 1 line

1.9.3

Amit Shah

author

2014-07-16 11:32 UTC #1 lore raw

PATCH 1/2 virtio-serial: create a linked list of all active devices

To ensure two virtserialports don't get added to the system with the same 'name' parameter, we need to access all the ports on all the devices added, and compare the names.

We currently don't have a list of all VirtIOSerial devices added to the system. This commit adds a simple linked list in which devices are put when they're initialized, and removed when they go away.

Signed-off-by Amit Shah <[email protected]>

---
 hw/char/virtio-serial-bus.c       | 10 ++++++++++
 include/hw/virtio/virtio-serial.h |  2 ++
 2 files changed, 12 insertions(+)

hw/char/virtio-serial-bus.c +10 -0

index 07bebc0..8c26f4e 100644
@@ -26,6 +26,10 @@
 #include "hw/virtio/virtio-serial.h"
 #include "hw/virtio/virtio-access.h"
 
+struct VirtIOSerialDevices {
+    QLIST_HEAD(, VirtIOSerial) devices;
+} vserdevices;
+
 static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
 {
     VirtIOSerialPort *port;

@@ -975,6 +979,8 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
      */
     register_savevm(dev, "virtio-console", -1, 3, virtio_serial_save,
                     virtio_serial_load, vser);
+
+    QLIST_INSERT_HEAD(&vserdevices.devices, vser, next);
 }
 
 static void virtio_serial_port_class_init(ObjectClass *klass, void *data)

@@ -1003,6 +1009,8 @@ static void virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOSerial *vser = VIRTIO_SERIAL(dev);
 
+    QLIST_REMOVE(vser, next);
+
     unregister_savevm(dev, "virtio-console", vser);
 
     g_free(vser->ivqs);

@@ -1027,6 +1035,8 @@ static void virtio_serial_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
 
+    QLIST_INIT(&vserdevices.devices);
+
     dc->props = virtio_serial_properties;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
     vdc->realize = virtio_serial_device_realize;

include/hw/virtio/virtio-serial.h +2 -0

index 4746312..a679e54 100644
@@ -202,6 +202,8 @@ struct VirtIOSerial {
 
     QTAILQ_HEAD(, VirtIOSerialPort) ports;
 
+    QLIST_ENTRY(VirtIOSerial) next;
+
     /* bitmap for identifying active ports */
     uint32_t *ports_map;

Signature · 1 line

1.9.3

Amit Shah

author

2014-07-16 11:32 UTC #9 lore raw

PATCH 2/2 virtio-serial: search for duplicate port names before adding new ports

In reply to Amit Shah

Before adding new ports to VirtIOSerial devices, check if there's a conflict in the 'name' parameter. This ensures two virtserialports with identical names are not initialized.

Reported-by <[email protected]>

Signed-off-by Amit Shah <[email protected]>

---
 hw/char/virtio-serial-bus.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

hw/char/virtio-serial-bus.c +22 -0

index 8c26f4e..76e4228 100644
@@ -56,6 +56,22 @@ static VirtIOSerialPort *find_port_by_vq(VirtIOSerial *vser, VirtQueue *vq)
     return NULL;
 }
 
+static VirtIOSerialPort *find_port_by_name(char *name)
+{
+    VirtIOSerial *vser;
+
+    QLIST_FOREACH(vser, &vserdevices.devices, next) {
+        VirtIOSerialPort *port;
+
+        QTAILQ_FOREACH(port, &vser->ports, next) {
+            if (!strcmp(port->name, name)) {
+                return port;
+            }
+        }
+    }
+    return NULL;
+}
+
 static bool use_multiport(VirtIOSerial *vser)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(vser);

@@ -847,6 +863,12 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (find_port_by_name(port->name)) {
+        error_setg(errp, "virtio-serial-bus: A port already exists by name %s",
+                   port->name);
+        return;
+    }
+
     if (port->id == VIRTIO_CONSOLE_BAD_ID) {
         if (plugging_port0) {
             port->id = 0;

Signature · 1 line

1.9.3

Markus Armbruster

2014-08-04 11:34 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 1/2] virtio-serial: create a linked list of all active devices

In reply to Amit Shah

Quoted 19 lines · Amit Shah

To ensure two virtserialports don't get added to the system with the same 'name' parameter, we need to access all the ports on all the devices added, and compare the names.
We currently don't have a list of all VirtIOSerial devices added to the system. This commit adds a simple linked list in which devices are put when they're initialized, and removed when they go away.
Signed-off-by Amit Shah <[email protected]>
---
 hw/char/virtio-serial-bus.c       | 10 ++++++++++
 include/hw/virtio/virtio-serial.h |  2 ++
 2 files changed, 12 insertions(+)
hw/char/virtio-serial-bus.c +0 -0
index 07bebc0..8c26f4e 100644

hw/char/virtio-serial-bus.c line 32

Quoted 7 lines

hw/char/virtio-serial-bus.c +4 -0

@@ -26,6 +26,10 @@
 #include "hw/virtio/virtio-serial.h"
 #include "hw/virtio/virtio-access.h"
 
+struct VirtIOSerialDevices {
+    QLIST_HEAD(, VirtIOSerial) devices;
+} vserdevices;
+

Any particular reason for stuffing the list into a struct?

Quoted 35 lines

hw/char/virtio-serial-bus.c +6 -0

 static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
 {
     VirtIOSerialPort *port;

@@ -975,6 +979,8 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
      */
     register_savevm(dev, "virtio-console", -1, 3, virtio_serial_save,
                     virtio_serial_load, vser);
+
+    QLIST_INSERT_HEAD(&vserdevices.devices, vser, next);
 }
 
 static void virtio_serial_port_class_init(ObjectClass *klass, void *data)

@@ -1003,6 +1009,8 @@ static void virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOSerial *vser = VIRTIO_SERIAL(dev);
 
+    QLIST_REMOVE(vser, next);
+
     unregister_savevm(dev, "virtio-console", vser);
 
     g_free(vser->ivqs);

@@ -1027,6 +1035,8 @@ static void virtio_serial_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
 
+    QLIST_INIT(&vserdevices.devices);
+
     dc->props = virtio_serial_properties;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
     vdc->realize = virtio_serial_device_realize;

include/hw/virtio/virtio-serial.h +0 -0

index 4746312..a679e54 100644

include/hw/virtio/virtio-serial.h struct VirtIOSerial { · line 208

Quoted 7 lines

include/hw/virtio/virtio-serial.h +2 -0

@@ -202,6 +202,8 @@ struct VirtIOSerial {
 
     QTAILQ_HEAD(, VirtIOSerialPort) ports;
 
+    QLIST_ENTRY(VirtIOSerial) next;
+
     /* bitmap for identifying active ports */
     uint32_t *ports_map;

Patch looks simple & safe to me, but I can't help to wonder whether want (or already have?) more generic infrastructure offering "for all devices of a certain kind" functionality, which is what 2/2 needs. Andreas?

Amit Shah

author

2014-08-04 11:45 UTC #3 lore raw

Re: [Qemu-devel] [PATCH 1/2] virtio-serial: create a linked list of all active devices

Quoted 9 lines · Markus Armbruster

Quoted 7 lines · Amit Shah
To ensure two virtserialports don't get added to the system with the same 'name' parameter, we need to access all the ports on all the devices added, and compare the names.
We currently don't have a list of all VirtIOSerial devices added to the system. This commit adds a simple linked list in which devices are put when they're initialized, and removed when they go away.

<snip>

Quoted 6 lines

hw/char/virtio-serial-bus.c line 32
Quoted 4 lines
hw/char/virtio-serial-bus.c +4 -0
+struct VirtIOSerialDevices {
+    QLIST_HEAD(, VirtIOSerial) devices;
+} vserdevices;
+
Any particular reason for stuffing the list into a struct?

Not strictly needed for this patch alone, but I think it's cleaner to keep it this way, and when something else comes up that needs a per-device variable, this list will be around. Also, this is also the way it's done in the kernel, so that uniformity helps as well.

<snip>

Quoted 3 lines

Patch looks simple & safe to me, but I can't help to wonder whether want (or already have?) more generic infrastructure offering "for all devices of a certain kind" functionality, which is what 2/2 needs. Andreas?

Yea; I didn't find any, but if there's already something it can be put to good use here.

Thanks,

Amit

Markus Armbruster

2014-08-06 07:27 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 1/2] virtio-serial: create a linked list of all active devices

Quoted 20 lines · Amit Shah

Quoted 9 lines · Markus Armbruster
Quoted 7 lines · Amit Shah
To ensure two virtserialports don't get added to the system with the same 'name' parameter, we need to access all the ports on all the devices added, and compare the names.
We currently don't have a list of all VirtIOSerial devices added to the system. This commit adds a simple linked list in which devices are put when they're initialized, and removed when they go away.
<snip>
Quoted 6 lines
hw/char/virtio-serial-bus.c line 32
Quoted 4 lines
hw/char/virtio-serial-bus.c +4 -0
+struct VirtIOSerialDevices {
+    QLIST_HEAD(, VirtIOSerial) devices;
+} vserdevices;
+
Any particular reason for stuffing the list into a struct?

Quoted 3 lines

Not strictly needed for this patch alone, but I think it's cleaner to keep it this way, and when something else comes up that needs a per-device variable, this list will be around.

Adding the struct when it's needed will be easy, so why pay the notational overhead now?

Quoted 2 lines

Also, this is also the
way it's done in the kernel, so that uniformity helps as well.

Two uglies make a pretty?

Anyway, matter of taste.

[...]

Markus Armbruster

Reviewed-by on the v1 series

2014-08-06 07:27 UTC #10 lore raw

In reply to Amit Shah

Quoted 11 lines · Amit Shah

Hi,
The 2nd patch prevents adding ports to the system with conflicts in the 'name' parameter. This is done by going through all the ports in the system, including ports on other virtio-serial devices.
The first patch prepares for this by creating a linked list of all available virtio-serial devices, so they can be walked over to check their ports' names.
Please review.

No advice from Andreas on better ways to iterate over devices, yet (vacation time?). If there is one, we can always use it on top.

Reviewed-by Markus Armbruster <[email protected]>

Amit Shah

author

2014-08-06 07:40 UTC #5 lore raw

Re: [Qemu-devel] [PATCH 1/2] virtio-serial: create a linked list of all active devices

In reply to Markus Armbruster

Quoted 22 lines · Markus Armbruster

Quoted 20 lines · Amit Shah
Quoted 9 lines · Markus Armbruster
Quoted 7 lines · Amit Shah
To ensure two virtserialports don't get added to the system with the same 'name' parameter, we need to access all the ports on all the devices added, and compare the names.
We currently don't have a list of all VirtIOSerial devices added to the system. This commit adds a simple linked list in which devices are put when they're initialized, and removed when they go away.
<snip>
Quoted 6 lines
hw/char/virtio-serial-bus.c line 32
Quoted 4 lines
hw/char/virtio-serial-bus.c +4 -0
+struct VirtIOSerialDevices {
+    QLIST_HEAD(, VirtIOSerial) devices;
+} vserdevices;
+
Any particular reason for stuffing the list into a struct?

Quoted 6 lines

Quoted 3 lines
Not strictly needed for this patch alone, but I think it's cleaner to keep it this way, and when something else comes up that needs a per-device variable, this list will be around.
Adding the struct when it's needed will be easy, so why pay the notational overhead now?

True.

Quoted 4 lines

Quoted 2 lines
Also, this is also the
way it's done in the kernel, so that uniformity helps as well.
Two uglies make a pretty?

Hah!

Amit

Amit Shah

author

2014-08-06 07:40 UTC #11 lore raw

In reply to Markus Armbruster

Quoted 12 lines · Markus Armbruster

Quoted 10 lines · Amit Shah
Hi,
The 2nd patch prevents adding ports to the system with conflicts in the 'name' parameter. This is done by going through all the ports in the system, including ports on other virtio-serial devices.
The first patch prepares for this by creating a linked list of all available virtio-serial devices, so they can be walked over to check their ports' names.

Quoted 6 lines

Quoted 1 line
Please review.
No advice from Andreas on better ways to iterate over devices, yet (vacation time?). If there is one, we can always use it on top.
Reviewed-by Markus Armbruster <[email protected]>

Thanks!

I'll wait for a week before submitting a pull req.

Amit

Amit Shah

author

2014-08-18 17:12 UTC #12 lore raw

In reply to Markus Armbruster

Quoted 8 lines · Markus Armbruster

Quoted 6 lines · Amit Shah
Hi,
The 2nd patch prevents adding ports to the system with conflicts in the 'name' parameter. This is done by going through all the ports in the system, including ports on other virtio-serial devices.

Quoted 8 lines

Quoted 5 lines
The first patch prepares for this by creating a linked list of all available virtio-serial devices, so they can be walked over to check their ports' names.
Please review.
No advice from Andreas on better ways to iterate over devices, yet (vacation time?). If there is one, we can always use it on top.

Couple of weeks later, no other responses, so will send a pull req.

Quoted 1 line

Reviewed-by Markus Armbruster <[email protected]>

Thanks!

Amit

Andreas Färber

2014-08-18 17:18 UTC #13 lore raw

Am 18.08.2014 19:10, schrieb Amit Shah:

Quoted 9 lines · Markus Armbruster

Quoted 8 lines
Quoted 6 lines · Amit Shah
Hi,
The 2nd patch prevents adding ports to the system with conflicts in the 'name' parameter. This is done by going through all the ports in the system, including ports on other virtio-serial devices.

Quoted 8 lines

Quoted 8 lines
Quoted 5 lines
The first patch prepares for this by creating a linked list of all available virtio-serial devices, so they can be walked over to check their ports' names.
Please review.
No advice from Andreas on better ways to iterate over devices, yet (vacation time?). If there is one, we can always use it on top.

I'm not aware of such a question. Context?

Quoted 1 line

Couple of weeks later, no other responses, so will send a pull req.

If you have a question, ask me on #qemu or put it on the call agenda. It's hopeless waiting for me to catch up with my old unread mail as new ones keep coming in.

Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Andreas Färber

2014-08-18 17:21 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 1/2] virtio-serial: create a linked list of all active devices

In reply to Amit Shah

Am 04.08.2014 13:45, schrieb Amit Shah:

Quoted 10 lines · Markus Armbruster

Quoted 9 lines
Quoted 7 lines · Amit Shah
To ensure two virtserialports don't get added to the system with the same 'name' parameter, we need to access all the ports on all the devices added, and compare the names.
We currently don't have a list of all VirtIOSerial devices added to the system. This commit adds a simple linked list in which devices are put when they're initialized, and removed when they go away.

[...]

Quoted 6 lines

Quoted 3 lines
Patch looks simple & safe to me, but I can't help to wonder whether want (or already have?) more generic infrastructure offering "for all devices of a certain kind" functionality, which is what 2/2 needs. Andreas?
Yea; I didn't find any, but if there's already something it can be put to good use here.

If you're looking for devices of the same QOM type, that can be done through some QOM path syntax that Paolo introduced. Someone used it in a recent iommu series.

Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Markus Armbruster

2014-08-19 15:46 UTC #7 lore raw

Re: [Qemu-devel] [PATCH 1/2] virtio-serial: create a linked list of all active devices

Quoted 16 lines · Andreas Färber

Am 04.08.2014 13:45, schrieb Amit Shah:
Quoted 10 lines · Markus Armbruster
Quoted 9 lines
Quoted 7 lines · Amit Shah
To ensure two virtserialports don't get added to the system with the same 'name' parameter, we need to access all the ports on all the devices added, and compare the names.
We currently don't have a list of all VirtIOSerial devices added to the system. This commit adds a simple linked list in which devices are put when they're initialized, and removed when they go away.
[...]
Quoted 4 lines
Quoted 3 lines
Patch looks simple & safe to me, but I can't help to wonder whether want (or already have?) more generic infrastructure offering "for all devices of a certain kind" functionality, which is what 2/2 needs. Andreas?

Quoted 6 lines

Quoted 2 lines
Yea; I didn't find any, but if there's already something it can be put to good use here.
If you're looking for devices of the same QOM type, that can be done through some QOM path syntax that Paolo introduced. Someone used it in a recent iommu series.

Possibly

    object_class_foreach(fn, TYPE_VIRTIO_SERIAL_PORT, false, false, arg);

Andreas Färber

2014-08-19 16:15 UTC #8 lore raw

Re: [Qemu-devel] [PATCH 1/2] virtio-serial: create a linked list of all active devices

Am 19.08.2014 17:45, schrieb Markus Armbruster:

Quoted 21 lines

Quoted 19 lines · Andreas Färber
Am 04.08.2014 13:45, schrieb Amit Shah:
Quoted 10 lines · Markus Armbruster
Quoted 9 lines
Quoted 7 lines · Amit Shah
To ensure two virtserialports don't get added to the system with the same 'name' parameter, we need to access all the ports on all the devices added, and compare the names.
We currently don't have a list of all VirtIOSerial devices added to the system. This commit adds a simple linked list in which devices are put when they're initialized, and removed when they go away.
[...]
Quoted 6 lines
Quoted 3 lines
Patch looks simple & safe to me, but I can't help to wonder whether want (or already have?) more generic infrastructure offering "for all devices of a certain kind" functionality, which is what 2/2 needs. Andreas?
Yea; I didn't find any, but if there's already something it can be put to good use here.

Quoted 7 lines

Quoted 3 lines
If you're looking for devices of the same QOM type, that can be done through some QOM path syntax that Paolo introduced. Someone used it in a recent iommu series.
Possibly
    object_class_foreach(fn, TYPE_VIRTIO_SERIAL_PORT, false, false, arg);

That enumerates derived types (ObjectClass), not Object instances.

Maybe it makes sense to add such an API for objects, too. However the only "registry" of objects we have is the QOM composition tree, i.e. it would be a tree-walking function.

Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg