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

On Wed, Oct 13, 2021 at 03:10:38PM +0200, Damien Hedde wrote: > > > @@ -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. Indeed. If we expected qdev_set_id() to be passed something that it can later free, we would have used 'char *'; but because we used 'const char *' for that parameter, it really does make more sense for the callers to pass in any string and for qdev_set_id() to do the necessary strdup()ing, as well as clean up on error.