From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJLCf-00060I-8u for qemu-devel@nongnu.org; Thu, 05 Feb 2015 07:06:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJLCa-0005uh-Vt for qemu-devel@nongnu.org; Thu, 05 Feb 2015 07:06:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40840) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJLCa-0005uU-OQ for qemu-devel@nongnu.org; Thu, 05 Feb 2015 07:06:32 -0500 Date: Thu, 5 Feb 2015 13:06:29 +0100 From: "Michael S. Tsirkin" Message-ID: <20150205120629.GC30113@redhat.com> References: <1421744647-26844-1-git-send-email-armbru@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1421744647-26844-1-git-send-email-armbru@redhat.com> Subject: Re: [Qemu-devel] [PATCH] qdev: Don't exit when running into bad -global List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: imammedo@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, afaerber@suse.de On Tue, Jan 20, 2015 at 10:04:07AM +0100, Markus Armbruster wrote: > -global lets you set a nice booby-trap for yourself: > > $ qemu-system-x86_64 -nodefaults -S -display none -usb -monitor stdio -global usb-mouse.usb_version=l > QEMU 2.1.94 monitor - type 'help' for more information > (qemu) device_add usb-mouse > Parameter 'usb_version' expects an int64 value or range > $ echo $? > 1 > > Not nice. Until commit 3196270 we even abort()ed. > > The same error triggers if you manage to screw up a machine type's > compat_props. To demonstrate, change HW_COMPAT_2_1's entry to > > .driver = "usb-mouse",\ > .property = "usb_version",\ > .value = "1", \ > > Then run > > $ qemu-system-x86_64 -usb -M pc-i440fx-2.1 -device usb-mouse > upstream-qemu: -device usb-mouse: Parameter 'usb_version' expects an int64 value or range > $ echo $? > 1 > > One of our creatively cruel error messages. > > Since this is actually a coding error, we *should* abort() here. > Replace the error by an assertion failure in this case. > > But turn the fatal error into a mere warning when the faulty > GlobalProperty comes from the user. Looks like this: > > $ qemu-system-x86_64 -nodefaults -S -display none -usb -monitor stdio -global usb-mouse.usb_version=l > QEMU 2.1.94 monitor - type 'help' for more information > (qemu) device_add usb-mouse > Warning: global usb-mouse.usb_version=l ignored (Parameter 'usb_version' expects an int64 value or range) > (qemu) > > This is consistent with how we handle similarly unusable -global in > qdev_prop_check_globals(). > > You could argue that the error should make device_add fail. Would be > harder, because we're running within TypeInfo's instance_post_init() > method device_post_init(), which can't fail. > > Signed-off-by: Markus Armbruster Applied, thanks! > --- > hw/core/qdev-properties.c | 21 +++++++++------------ > hw/core/qdev.c | 8 +------- > include/hw/qdev-properties.h | 4 +--- > 3 files changed, 11 insertions(+), 22 deletions(-) > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 2e47f70..5a4e4d5 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -990,8 +990,8 @@ int qdev_prop_check_globals(void) > return ret; > } > > -void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, > - Error **errp) > +static void qdev_prop_set_globals_for_type(DeviceState *dev, > + const char *typename) > { > GlobalProperty *prop; > > @@ -1004,25 +1004,22 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, > prop->used = true; > object_property_parse(OBJECT(dev), prop->value, prop->property, &err); > if (err != NULL) { > - error_propagate(errp, err); > + assert(prop->user_provided); > + error_report("Warning: global %s.%s=%s ignored (%s)", > + prop->driver, prop->property, prop->value, > + error_get_pretty(err)); > + error_free(err); > return; > } > } > } > > -void qdev_prop_set_globals(DeviceState *dev, Error **errp) > +void qdev_prop_set_globals(DeviceState *dev) > { > ObjectClass *class = object_get_class(OBJECT(dev)); > > do { > - Error *err = NULL; > - > - qdev_prop_set_globals_for_type(dev, object_class_get_name(class), > - &err); > - if (err != NULL) { > - error_propagate(errp, err); > - return; > - } > + qdev_prop_set_globals_for_type(dev, object_class_get_name(class)); > class = object_class_get_parent(class); > } while (class); > } > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 901f289..827c084 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -1126,13 +1126,7 @@ static void device_initfn(Object *obj) > > static void device_post_init(Object *obj) > { > - Error *err = NULL; > - qdev_prop_set_globals(DEVICE(obj), &err); > - if (err) { > - qerror_report_err(err); > - error_free(err); > - exit(EXIT_FAILURE); > - } > + qdev_prop_set_globals(DEVICE(obj)); > } > > /* Unlink device from bus and free the structure. */ > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h > index 070006c..57ee363 100644 > --- a/include/hw/qdev-properties.h > +++ b/include/hw/qdev-properties.h > @@ -180,9 +180,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value); > void qdev_prop_register_global(GlobalProperty *prop); > void qdev_prop_register_global_list(GlobalProperty *props); > int qdev_prop_check_globals(void); > -void qdev_prop_set_globals(DeviceState *dev, Error **errp); > -void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, > - Error **errp); > +void qdev_prop_set_globals(DeviceState *dev); > void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev, > Property *prop, const char *value); > > -- > 1.9.3