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

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?

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

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.

[...]

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

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

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

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