Re: [Qemu-devel] [PATCH v2] qdev-monitor.c: Add device id generation

[Daniel P. Berrange]

On Thu, Aug 27, 2015 at 07:14:28PM -0400, Programmingkid wrote:
> Add device ID generation to each device if an ID isn't given.
> 
> An auto-generated ID will begin with an underscore character. This will
> distinguish it from user-made ID's. 
> 
> An user-made ID cannot begin with an underscore, so there is no
> problem of collisions with auto-generated ID's. 
> 
> A management program like libvirt should have no problems with this
> patch since the names for ID's it gives do not start with an underscore. 
> 
> Signed-off-by: John Arbuckle <address@hidden>
> 
> ---
>  qdev-monitor.c |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index f9e2d62..a9beb6d 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -574,18 +574,17 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
> **errp)
>      id = qemu_opts_id(opts);
>      if (id) {
>          dev->id = id;
> +    } else { /* create an ID for a device if none is provided */
> +        static uint64_t deviceIDCount;
> +        char *deviceIDString;
> +
> +        /* Add an underscore to show this ID is auto-generated */
> +        deviceIDString = g_strdup_printf("_%" PRIu64, deviceIDCount++);
> +        dev->id = (const char *) deviceIDString;

The ID namespace is global to QOM, but this counter is scoped to the
qdev code, so we should have a further prefix, in case other bits of
code that also use QOM need to have auto-generated IDs.

IOW I think we should use

  deviceIDString = g_strdup_printf("_qdev%" PRIu64, deviceIDCount++);

>      }
>  
> -    if (dev->id) {
> -        object_property_add_child(qdev_get_peripheral(), dev->id,
> -                                  OBJECT(dev), NULL);
> -    } else {
> -        static int anon_count;
> -        gchar *name = g_strdup_printf("device[%d]", anon_count++);
> -        object_property_add_child(qdev_get_peripheral_anon(), name,
> +    object_property_add_child(qdev_get_peripheral(), dev->id,
>                                    OBJECT(dev), NULL);
> -        g_free(name);
> -    }

It looks like this is removing the only use of the /machine/peripheral-anon
part of the tre, so if we do this, then we should remove the refernces to
peripheral-anon from the rest of the codebase. Alternatively we could keep
devices with auto-generated IDs under /peripheral-anon, but I don't really
see much point. The only reason this was separated was to avoid the
auto-generated path name clashing with a user provided name, but we avoid
that now by using an underscore. So we might as well just kill off the
/peripheral-anon part of the tree and leave just /peripheral 

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|