Re: [PATCH 06/11] qdev: Add Error parameter to qdev_set_id()

[Damien Hedde <damien.hedde@greensocs.com>]

Hi Kevin,

I proposed a very similar patch in our rfc series because we needed some 
of the cleaning you do here.
https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05679.html
I've added a bit of doc for the function, feel free to take it if you want.

On 9/24/21 16:09, Vladimir Sementsov-Ogievskiy wrote:
> 24.09.2021 12:04, Kevin Wolf wrote:
>> object_property_add_child() fails (with &error_abort) if an object with
>> the same name already exists. As long as QemuOpts is in use for -device
>> and device_add, it catches duplicate IDs before qdev_set_id() is even
>> called. However, for enabling non-QemuOpts code paths, we need to make
>> sure that the condition doesn't cause a crash, but fails gracefully.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   include/monitor/qdev.h      |  2 +-
>>   hw/xen/xen-legacy-backend.c |  3 ++-
>>   softmmu/qdev-monitor.c      | 16 ++++++++++------
>>   3 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
>> index 389287eb44..7961308c75 100644
>> --- a/include/monitor/qdev.h
>> +++ b/include/monitor/qdev.h
>> @@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, 
>> Error **errp);
>>   int qdev_device_help(QemuOpts *opts);
>>   DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
>> -void qdev_set_id(DeviceState *dev, char *id);
>> +void qdev_set_id(DeviceState *dev, char *id, Error **errp);
>>   #endif
>> diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
>> index dd8ae1452d..17aca85ddc 100644
>> --- a/hw/xen/xen-legacy-backend.c
>> +++ b/hw/xen/xen-legacy-backend.c
>> @@ -276,7 +276,8 @@ static struct XenLegacyDevice 
>> *xen_be_get_xendev(const char *type, int dom,
>>       xendev = g_malloc0(ops->size);
>>       object_initialize(&xendev->qdev, ops->size, TYPE_XENBACKEND);
>>       OBJECT(xendev)->free = g_free;
>> -    qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, 
>> dev));
>> +    qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
>> +                &error_abort);
>>       qdev_realize(DEVICE(xendev), xen_sysbus, &error_fatal);
>>       object_unref(OBJECT(xendev));
>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> index 1207e57a46..c2af906df0 100644
>> --- a/softmmu/qdev-monitor.c
>> +++ b/softmmu/qdev-monitor.c
>> @@ -593,26 +593,27 @@ static BusState *qbus_find(const char *path, 
>> Error **errp)
>>   }
>>   /* Takes ownership of @id, will be freed when deleting the device */
>> -void qdev_set_id(DeviceState *dev, char *id)
>> +void qdev_set_id(DeviceState *dev, char *id, Error **errp)
> 
> According to recommendations in error.h, worth adding also return value 
> (for example true=success false=failure), so [..]
> 
>>   {
>>       if (id) {
>>           dev->id = id;
>>       }
> 
> Unrelated but.. What's the strange logic?
> 
> Is it intended that with passed id=NULL we don't update dev->id variable 
> but try to do following logic with old dev->id?

dev->id is expected to be NULL. The object is created just before 
calling this function so it is always the case. We could probably assert 
this.

> 
>>       if (dev->id) {
>> -        object_property_add_child(qdev_get_peripheral(), dev->id,
>> -                                  OBJECT(dev));
>> +        object_property_try_add_child(qdev_get_peripheral(), dev->id,
>> +                                      OBJECT(dev), errp);
>>       } else {
>>           static int anon_count;
>>           gchar *name = g_strdup_printf("device[%d]", anon_count++);
>> -        object_property_add_child(qdev_get_peripheral_anon(), name,
>> -                                  OBJECT(dev));
>> +        object_property_try_add_child(qdev_get_peripheral_anon(), name,
>> +                                      OBJECT(dev), errp);
>>           g_free(name);
>>       }
>>   }
>>   DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>   {
>> +    ERRP_GUARD();
>>       DeviceClass *dc;
>>       const char *driver, *path;
>>       DeviceState *dev = NULL;
>> @@ -691,7 +692,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, 
>> Error **errp)
>>           }
>>       }
>> -    qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
>> +    qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp);
>> +    if (*errp) {
>> +        goto err_del_dev;
>> +    }
> 
> [..] here we'll have
> 
> if (!qdev_set_id(...)) {
>    goto err_del_dev;
> }
> 
> and no need for ERRP_GUARD.
> 
>>       /* set properties */
>>       if (qemu_opt_foreach(opts, set_property, dev, errp)) {
>>
> 
>