From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56542) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wt1qy-0003el-QY for qemu-devel@nongnu.org; Fri, 06 Jun 2014 17:39:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wt1qr-0002vL-6i for qemu-devel@nongnu.org; Fri, 06 Jun 2014 17:39:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30047) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wt1qq-0002tf-VS for qemu-devel@nongnu.org; Fri, 06 Jun 2014 17:39:05 -0400 Date: Fri, 6 Jun 2014 23:38:58 +0200 From: Igor Mammedov Message-ID: <20140606233858.1f126642@thinkpad> In-Reply-To: <20140606201429.GK15000@otherpad.lan.raisama.net> References: <20140606201429.GK15000@otherpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qdev: Skip non-existing properties when setting globals List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, Don Slutz , Markus Armbruster , Paolo Bonzini , Andreas =?ISO-8859-1?B?RuRyYmVy?= On Fri, 6 Jun 2014 17:14:29 -0300 Eduardo Habkost wrote: > This avoids QEMU from aborting on cases like this: > > $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5 > qemu-system-x86_64: Property '.foobar' not found > Aborted (core dumped) That is expected behavior. > > The code sets dev->not_used if the property is not found as an effort to > to allow errors to be reported even if the device is hotpluggable, but > it won't catch all errors. We can't know the property is not going to be > available for hotpluggable devices, unless we actually try to create the > device. Instead of ignoring users errors, DeviceState should have async_error field which could be set by device_post_init() instead of aborting and later device_add could gracefully fail hotadd operation if error is set. PS: initfn-s could also reuse this, instead of ignoring errors as they do now. > > Signed-off-by: Eduardo Habkost > --- > hw/core/qdev-properties.c | 10 +++++++++- > tests/test-qdev-global-props.c | 14 ++++++++++++-- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 3d12560..8cd7c2a 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -976,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, > Error **errp) > { > GlobalProperty *prop; > + Object *obj = OBJECT(dev); > > QTAILQ_FOREACH(prop, &global_props, next) { > Error *err = NULL; > @@ -983,8 +984,15 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, > if (strcmp(typename, prop->driver) != 0) { > continue; > } > + if (!object_property_find(obj, prop->property, &err)) { > + /* not_used doesn't default to true for hotpluggable devices, > + * but in this case we know the property wasn't used when it could. > + */ > + prop->not_used = true; > + continue; > + } > prop->not_used = false; > - object_property_parse(OBJECT(dev), prop->value, prop->property, &err); > + object_property_parse(obj, prop->value, prop->property, &err); > if (err != NULL) { > error_propagate(errp, err); > return; > diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c > index 2bef04c..1ccc3e5 100644 > --- a/tests/test-qdev-global-props.c > +++ b/tests/test-qdev-global-props.c > @@ -148,8 +148,9 @@ static void test_dynamic_globalprop(void) > { > MyType *mt; > static GlobalProperty props[] = { > - { TYPE_DYNAMIC_PROPS, "prop1", "101" }, > - { TYPE_DYNAMIC_PROPS, "prop2", "102" }, > + { TYPE_DYNAMIC_PROPS, "prop1", "101", true }, > + { TYPE_DYNAMIC_PROPS, "prop2", "102", true }, > + { TYPE_DYNAMIC_PROPS, "prop3", "103", true }, > { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true }, > {} > }; > @@ -164,6 +165,15 @@ static void test_dynamic_globalprop(void) > g_assert_cmpuint(mt->prop2, ==, 102); > all_used = qdev_prop_check_global(); > g_assert_cmpuint(all_used, ==, 1); > + > + /* prop1 */ > + g_assert(!props[0].not_used); > + /* prop2 */ > + g_assert(!props[1].not_used); > + /* TYPE_DYNAMIC_PROPS.prop3: non-existing property */ > + g_assert(props[2].not_used); > + /* TYPE_DYNAMIC_PROPS-bad.prop3: non-existing class */ > + g_assert(props[3].not_used); > } > > int main(int argc, char **argv) > -- > 1.9.0 > > -- Regards, Igor