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

[Eric Blake]

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org