[Qemu-devel] [PATCH 0/2] virtio-serial: Prevent addition of ports with same name

[Qemu-devel] [PATCH 0/2] virtio-serial: Prevent addition of ports with same name

[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.

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(+)

-- 
1.9.3

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

[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 <amit.shah@redhat.com>
---
 hw/char/virtio-serial-bus.c       | 10 ++++++++++
 include/hw/virtio/virtio-serial.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 07bebc0..8c26f4e 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -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;
diff --git a/include/hw/virtio/virtio-serial.h b/include/hw/virtio/virtio-serial.h
index 4746312..a679e54 100644
--- a/include/hw/virtio/virtio-serial.h
+++ b/include/hw/virtio/virtio-serial.h
@@ -202,6 +202,8 @@ struct VirtIOSerial {
 
     QTAILQ_HEAD(, VirtIOSerialPort) ports;
 
+    QLIST_ENTRY(VirtIOSerial) next;
+
     /* bitmap for identifying active ports */
     uint32_t *ports_map;
 
-- 
1.9.3

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

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> 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 <amit.shah@redhat.com>
> ---
>  hw/char/virtio-serial-bus.c       | 10 ++++++++++
>  include/hw/virtio/virtio-serial.h |  2 ++
>  2 files changed, 12 insertions(+)
>
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 07bebc0..8c26f4e 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -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?

>  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;
> diff --git a/include/hw/virtio/virtio-serial.h b/include/hw/virtio/virtio-serial.h
> index 4746312..a679e54 100644
> --- a/include/hw/virtio/virtio-serial.h
> +++ b/include/hw/virtio/virtio-serial.h
> @@ -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?

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

[Amit Shah]

On (Mon) 04 Aug 2014 [13:33:56], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > 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>

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

> 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

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

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Mon) 04 Aug 2014 [13:33:56], Markus Armbruster wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>> 
>> > 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>
>
>> > +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.

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

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

[...]

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

[Amit Shah]

On (Wed) 06 Aug 2014 [09:27:02], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Mon) 04 Aug 2014 [13:33:56], Markus Armbruster wrote:
> >> Amit Shah <amit.shah@redhat.com> writes:
> >> 
> >> > 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>
> >
> >> > +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.
> 
> Adding the struct when it's needed will be easy, so why pay the
> notational overhead now?

True.

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

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

[Andreas Färber]

Am 04.08.2014 13:45, schrieb Amit Shah:
> On (Mon) 04 Aug 2014 [13:33:56], Markus Armbruster wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>>
>>> 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.
[...]
>> 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

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

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

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 04.08.2014 13:45, schrieb Amit Shah:
>> On (Mon) 04 Aug 2014 [13:33:56], Markus Armbruster wrote:
>>> Amit Shah <amit.shah@redhat.com> writes:
>>>
>>>> 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.
> [...]
>>> 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.

Possibly

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

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

[Andreas Färber]

Am 19.08.2014 17:45, schrieb Markus Armbruster:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 04.08.2014 13:45, schrieb Amit Shah:
>>> On (Mon) 04 Aug 2014 [13:33:56], Markus Armbruster wrote:
>>>> Amit Shah <amit.shah@redhat.com> writes:
>>>>
>>>>> 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.
>> [...]
>>>> 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.
> 
> 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

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

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

[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: <mazhang@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/char/virtio-serial-bus.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 8c26f4e..76e4228 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -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;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 0/2] virtio-serial: Prevent addition of ports with same name

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> 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 <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] virtio-serial: Prevent addition of ports with same name

[Amit Shah]

On (Wed) 06 Aug 2014 [09:27:04], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > 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 <armbru@redhat.com>

Thanks!

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


		Amit

Re: [Qemu-devel] [PATCH 0/2] virtio-serial: Prevent addition of ports with same name

[Amit Shah]

On (Wed) 06 Aug 2014 [09:27:04], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > 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.

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

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

		Amit

Re: [Qemu-devel] [PATCH 0/2] virtio-serial: Prevent addition of ports with same name

[Andreas Färber]

Am 18.08.2014 19:10, schrieb Amit Shah:
> On (Wed) 06 Aug 2014 [09:27:04], Markus Armbruster wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>>
>>> 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.

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

> 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

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