From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60522) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9yCl-0003UO-55 for qemu-devel@nongnu.org; Fri, 18 Dec 2015 11:48:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9yCh-0005v2-PU for qemu-devel@nongnu.org; Fri, 18 Dec 2015 11:48:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35639) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9yCh-0005uy-Ka for qemu-devel@nongnu.org; Fri, 18 Dec 2015 11:48:27 -0500 Date: Fri, 18 Dec 2015 16:48:23 +0000 From: "Daniel P. Berrange" Message-ID: <20151218164823.GH7228@redhat.com> References: <1450452647-118105-1-git-send-email-imammedo@redhat.com> <1450452647-118105-3-git-send-email-imammedo@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1450452647-118105-3-git-send-email-imammedo@redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/2] qdev: safely fail device_add if unable to allocate device Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, afaerber@suse.de 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 > --- > 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 :|