From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36476) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFzQx-00008Q-Sm for qemu-devel@nongnu.org; Thu, 23 Jun 2016 03:52:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFzQt-000746-OI for qemu-devel@nongnu.org; Thu, 23 Jun 2016 03:52:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59319) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFzQt-00073u-Gu for qemu-devel@nongnu.org; Thu, 23 Jun 2016 03:52:15 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C61F981F07 for ; Thu, 23 Jun 2016 07:52:14 +0000 (UTC) From: Markus Armbruster References: <1466437983-27133-1-git-send-email-ehabkost@redhat.com> <1466437983-27133-4-git-send-email-ehabkost@redhat.com> Date: Thu, 23 Jun 2016 09:52:12 +0200 In-Reply-To: <1466437983-27133-4-git-send-email-ehabkost@redhat.com> (Eduardo Habkost's message of "Mon, 20 Jun 2016 12:52:56 -0300") Message-ID: <874m8k736b.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v2 03/10] vl: Reject invalid class names on -global List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, Marcel Apfelbaum , Paolo Bonzini , Igor Mammedov Eduardo Habkost writes: > Instead of just printing a warning very late, reject obviously > invalid -global arguments by validating the class name. > > Update test-qdev-global-props to not expect class name validation > to be performed in qdev_prop_check_globals(). > > Reviewed-by: Markus Armbruster > Signed-off-by: Eduardo Habkost > --- > Changes v1 -> v2: > * Fix test-qdev-global-props unit test > * Simplify object_dynamic_cast() check > * Suggested-by: Markus Armbruster > --- > hw/core/qdev-properties.c | 7 ------- > tests/test-qdev-global-props.c | 10 ---------- > vl.c | 20 +++++++++++++++++--- > 3 files changed, 17 insertions(+), 20 deletions(-) > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index c10edee..64e17aa 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -1052,13 +1052,6 @@ int qdev_prop_check_globals(void) > continue; > } > oc = object_class_by_name(prop->driver); > - oc = object_class_dynamic_cast(oc, TYPE_DEVICE); > - if (!oc) { > - error_report("Warning: global %s.%s has invalid class name", > - prop->driver, prop->property); > - ret = 1; > - continue; > - } > dc = DEVICE_CLASS(oc); > if (!dc->hotpluggable && !prop->used) { > error_report("Warning: global %s.%s=%s not used", > diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c > index 48e5b73..db77ad9 100644 > --- a/tests/test-qdev-global-props.c > +++ b/tests/test-qdev-global-props.c > @@ -201,10 +201,8 @@ static void test_dynamic_globalprop_subprocess(void) > static GlobalProperty props[] = { > { TYPE_DYNAMIC_PROPS, "prop1", "101", true }, > { TYPE_DYNAMIC_PROPS, "prop2", "102", true }, > - { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true }, > { TYPE_UNUSED_HOTPLUG, "prop4", "104", true }, > { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", true }, > - { TYPE_NONDEVICE, "prop6", "106", true }, > {} > }; > int all_used; > @@ -222,8 +220,6 @@ static void test_dynamic_globalprop_subprocess(void) > g_assert(props[1].used); > g_assert(!props[2].used); > g_assert(!props[3].used); > - g_assert(!props[4].used); > - g_assert(!props[5].used); > } > > static void test_dynamic_globalprop(void) > @@ -232,10 +228,8 @@ static void test_dynamic_globalprop(void) > g_test_trap_assert_passed(); > g_test_trap_assert_stderr_unmatched("*prop1*"); > g_test_trap_assert_stderr_unmatched("*prop2*"); > - g_test_trap_assert_stderr("*Warning: global dynamic-prop-type-bad.prop3 has invalid class name\n*"); > g_test_trap_assert_stderr_unmatched("*prop4*"); > g_test_trap_assert_stderr("*Warning: global nohotplug-type.prop5=105 not used\n*"); > - g_test_trap_assert_stderr("*Warning: global nondevice-type.prop6 has invalid class name\n*"); > g_test_trap_assert_stdout(""); > } > > @@ -246,10 +240,8 @@ static void test_dynamic_globalprop_nouser_subprocess(void) > static GlobalProperty props[] = { > { TYPE_DYNAMIC_PROPS, "prop1", "101" }, > { TYPE_DYNAMIC_PROPS, "prop2", "102" }, > - { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103" }, > { TYPE_UNUSED_HOTPLUG, "prop4", "104" }, > { TYPE_UNUSED_NOHOTPLUG, "prop5", "105" }, > - { TYPE_NONDEVICE, "prop6", "106" }, > {} > }; > int all_used; > @@ -267,8 +259,6 @@ static void test_dynamic_globalprop_nouser_subprocess(void) > g_assert(props[1].used); > g_assert(!props[2].used); > g_assert(!props[3].used); > - g_assert(!props[4].used); > - g_assert(!props[5].used); > } > The rest of the patch turns a warning into an error, but the test update *drops* the affected test cases. Shouldn't we test the error instead? To keep my R-by, you can either do that, or mention the loss of test cases in the commit message. [...]