Re: [Qemu-devel] [PATCH 2/2] qdev: safely fail device_add if unable to allocate device

[Daniel P. Berrange]

On Fri, Dec 18, 2015 at 04:30:47PM +0100, Igor Mammedov wrote:
> qdev_device_add() currently uses object_new() which
> will abort if there memory allocation for device instance
> fails. While it's fine it startup, it is not desirable
> diring hotplug.
> 
> Try to allocate memory for object first and fail safely
> if allocation fails.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> It's just a step in making hotplug safer wrt object allocation.
> To make it more safer, hotplugged class constructor
> shouldn't allocate memory either, but that should be
> addressed on per device basis providing we fix QOM
> internals to avoid dynamic allocations.
> ---
>  qdev-monitor.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index a35098f..a70262e 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -514,6 +514,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>      DeviceClass *dc;
>      const char *driver, *path, *id;
>      DeviceState *dev;
> +    size_t obj_size;
>      BusState *bus = NULL;
>      Error *err = NULL;
>  
> @@ -555,7 +556,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
> **errp)
>      }
>  
>      /* create device */
> -    dev = DEVICE(object_new(driver));
> +    obj_size = object_class_get_instance_size(OBJECT_CLASS(dc));
> +    dev = g_try_malloc0(obj_size);
> +    if (dev == NULL) {
> +        error_setg(errp, "Not enough memory for Device '%s'", driver);
> +        return NULL;
> +    }

This just avoids one small malloc failure.

> +    object_initialize(dev, obj_size, driver);

This is going to call g_new many more times, so you'll
still hit OOM almost immediately. eg the call to
g_hash_table_new_full() in object_initialize_with_type
will abort on OOM, not to mention anything run in a
instance constructor function registered against the
class. There's no way to avoid this given that we have
chosen to use GLib in QEMU, so I don't really see any
point in replacing the 'object_new' call with g_try_malloc

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|