Re: [Qemu-devel] [PATCH 6/6] qdev: Generate IDs for anonymous devices

[Anthony Liguori]

On 08/26/2011 09:48 AM, Jan Kiszka wrote:
> In order to address devices for that the user forgot or is even unable
> (no_user) to provide an ID, assign an automatically generated one. Such
> IDs have the format #<number>, thus are outside the name space availing
> to users. Don't use them for bus naming to avoid any other user-visible
> change.

I don't think this is a very nice approach.  Why not eliminate anonymous 
devices entirely and use a parent derived name for devices that are not 
created by the user?

Regards,

Anthony Liguori

> CC: Markus Armbruster<address@hidden>
> Signed-off-by: Jan Kiszka<address@hidden>
> ---
>   hw/qdev-properties.c |    2 +-
>   hw/qdev.c            |   25 +++++++++++++------------
>   hw/qdev.h            |    1 +
>   3 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 0c0c292..4e39cef 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -682,7 +682,7 @@ int qdev_prop_set_drive(DeviceState *dev, const char *name, 
> BlockDriverState *va
>       if (res<  0) {
>           error_report("Can't attach drive %s to %s.%s: %s",
>                        bdrv_get_device_name(value),
> -                     dev->id ? dev->id : dev->info->name,
> +                     dev->id != dev->auto_id ? dev->id : dev->info->name,
>                        name, strerror(-res));
>           return -1;
>       }
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 3b0e1af..807902b 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -85,6 +85,7 @@ static DeviceInfo *qdev_find_info(BusInfo *bus_info, const 
> char *name)
>
>   static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
>   {
> +    static unsigned int auto_id;
>       DeviceState *dev;
>
>       assert(bus->info == info->bus_info);
> @@ -94,6 +95,8 @@ static DeviceState *qdev_create_from_info(BusState *bus, 
> DeviceInfo *info)
>       qdev_prop_set_defaults(dev, dev->info->props);
>       qdev_prop_set_defaults(dev, dev->parent_bus->info->props);
>       qdev_prop_set_globals(dev);
> +    snprintf(dev->auto_id, sizeof(dev->auto_id), "#%u", ++auto_id);
> +    dev->id = dev->auto_id;
>       QLIST_INSERT_HEAD(&bus->children, dev, sibling);
>       if (qdev_hotplug) {
>           assert(bus->allow_hotplug);
> @@ -574,8 +577,9 @@ DeviceState *qdev_find_recursive(BusState *bus, const char 
> *id)
>       BusState *child;
>
>       QLIST_FOREACH(dev,&bus->children, sibling) {
> -        if (dev->id&&  strcmp(dev->id, id) == 0)
> +        if (strcmp(dev->id, id) == 0) {
>               return dev;
> +        }
>           QLIST_FOREACH(child,&dev->child_bus, sibling) {
>               ret = qdev_find_recursive(child, id);
>               if (ret) {
> @@ -591,8 +595,8 @@ static void qbus_list_bus(DeviceState *dev)
>       BusState *child;
>       const char *sep = " ";
>
> -    error_printf("child busses at \"%s\":",
> -                 dev->id ? dev->id : dev->info->name);
> +    error_printf("child busses at \"%s\"/\"%s\":",
> +                 dev->info->name, dev->id);
>       QLIST_FOREACH(child,&dev->child_bus, sibling) {
>           error_printf("%s\"%s\"", sep, child->name);
>           sep = ", ";
> @@ -607,9 +611,7 @@ static void qbus_list_dev(BusState *bus)
>
>       error_printf("devices at \"%s\":", bus->name);
>       QLIST_FOREACH(dev,&bus->children, sibling) {
> -        error_printf("%s\"%s\"", sep, dev->info->name);
> -        if (dev->id)
> -            error_printf("/\"%s\"", dev->id);
> +        error_printf("%s\"%s\"/\"%s\"", sep, dev->info->name, dev->id);
>           sep = ", ";
>       }
>       error_printf("\n");
> @@ -638,7 +640,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>        *   (3) driver alias, if present
>        */
>       QLIST_FOREACH(dev,&bus->children, sibling) {
> -        if (dev->id&&   strcmp(dev->id, elem) == 0) {
> +        if (strcmp(dev->id, elem) == 0) {
>               return dev;
>           }
>       }
> @@ -754,8 +756,8 @@ void qbus_create_inplace(BusState *bus, BusInfo *info,
>       if (name) {
>           /* use supplied name */
>           bus->name = g_strdup(name);
> -    } else if (parent&&  parent->id) {
> -        /* parent device has id ->  use it for bus name */
> +    } else if (parent&&  parent->id != parent->auto_id) {
> +        /* parent device has user-assigned id ->  use it for bus name */
>           len = strlen(parent->id) + 16;
>           buf = g_malloc(len);
>           snprintf(buf, len, "%s.%d", parent->id, parent->num_child_bus);
> @@ -850,8 +852,7 @@ static void qdev_print_props(Monitor *mon, DeviceState 
> *dev, Property *props,
>   static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
>   {
>       BusState *child;
> -    qdev_printf("dev: %s, id \"%s\"\n", dev->info->name,
> -                dev->id ? dev->id : "");
> +    qdev_printf("dev: %s, id \"%s\"\n", dev->info->name, dev->id);
>       indent += 2;
>       if (dev->num_gpio_in) {
>           qdev_printf("gpio-in %d\n", dev->num_gpio_in);
> @@ -1196,7 +1197,7 @@ int do_device_show(Monitor *mon, const QDict *qdict, 
> QObject **ret_data)
>       name = g_malloc(name_len);
>       snprintf(name, name_len, "%s", dev->info->name);
>       *ret_data = qobject_from_jsonf("{ 'device': %s, 'id': %s, 'version': %d 
> }",
> -                                   name, dev->id ? : "", vmsd->version_id);
> +                                   name, dev->id, vmsd->version_id);
>       g_free(name);
>       qlist = qlist_new();
>       parse_vmstate(vmsd, dev, qlist, qdict_get_try_bool(qdict, "full", false));
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 912fc45..d028409 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -31,6 +31,7 @@ enum {
>      so that it can be embedded in individual device state structures.  */
>   struct DeviceState {
>       const char *id;
> +    char auto_id[16];
>       enum DevState state;
>       QemuOpts *opts;
>       int hotplugged;