[PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id

[Damien Hedde]

On 10/11/21 23:00, Eric Blake wrote:
> On Fri, Oct 08, 2021 at 03:34:36PM +0200, Kevin Wolf wrote:
>> From: Damien Hedde <damien.hedde at greensocs.com>
>>
>> qdev_set_id() is mostly used when the user adds a device (using
>> -device cli option or device_add qmp command). This commit adds
>> an error parameter to handle the case where the given id is
>> already taken.
>>
>> Also document the function and add a return value in order to
>> be able to capture success/failure: the function now returns the
>> id in case of success, or NULL in case of failure.
>>
>> The commit modifies the 2 calling places (qdev-monitor and
>> xen-legacy-backend) to add the error object parameter.
>>
>> Note that the id is, right now, guaranteed to be unique because
>> all ids came from the "device" QemuOptsList where the id is used
>> as key. This addition is a preparation for a future commit which
>> will relax the uniqueness.
>>
>> Signed-off-by: Damien Hedde <damien.hedde at greensocs.com>
>> Signed-off-by: Kevin Wolf <kwolf at redhat.com>
>> ---
> 
>> +        } else {
>> +            error_setg(errp, "Duplicate device ID '%s'", id);
>> +            return NULL;
> 
> id is not freed on this error path...
> 

>>   
>>   DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>> @@ -691,7 +703,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>           }
>>       }
>>   
>> -    qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
>> +    /*
>> +     * set dev's parent and register its id.
>> +     * If it fails it means the id is already taken.
>> +     */
>> +    if (!qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp)) {
>> +        goto err_del_dev;
> 
> ...nor on this, which means the g_strdup() leaks on failure.
> 

Since we strdup the id just before calling qdev_set_id.
Maybe we should do the the strdup in qdev_set_id (and free things on 
error there too). It seems simplier than freeing things on the callee 
side just in case of an error.


Damien