From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35069) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCwCN-0007TE-7v for qemu-devel@nongnu.org; Tue, 14 Jun 2016 17:48:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCwCL-0002lB-4p for qemu-devel@nongnu.org; Tue, 14 Jun 2016 17:48:38 -0400 Date: Tue, 14 Jun 2016 18:48:27 -0300 From: Eduardo Habkost Message-ID: <20160614214827.GE18662@thinpad.lan.raisama.net> References: <1465492263-28472-1-git-send-email-imammedo@redhat.com> <1465492263-28472-5-git-send-email-imammedo@redhat.com> <20160614194718.GJ17952@thinpad.lan.raisama.net> <1646409152.22421731.1465940463160.JavaMail.zimbra@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1646409152.22421731.1465940463160.JavaMail.zimbra@redhat.com> Subject: Re: [Qemu-devel] Handling errors caused by -global (was Re: [PATCH v2 4/6] cpu: use CPUClass->parse_features() as convertor to global properties) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Igor Mammedov , qemu-devel@nongnu.org, peter maydell , mark cave-ayland , blauwirbel@gmail.com, qemu-arm@nongnu.org, rth@twiddle.net, Markus Armbruster On Tue, Jun 14, 2016 at 05:41:03PM -0400, Paolo Bonzini wrote: > > > ----- Original Message ----- > > From: "Eduardo Habkost" > > To: "Igor Mammedov" > > Cc: qemu-devel@nongnu.org, "peter maydell" , "mark cave-ayland" > > , blauwirbel@gmail.com, qemu-arm@nongnu.org, pbonzini@redhat.com, rth@twiddle.net, > > "Markus Armbruster" > > Sent: Tuesday, June 14, 2016 9:47:18 PM > > Subject: Handling errors caused by -global (was Re: [Qemu-devel] [PATCH v2 4/6] cpu: use CPUClass->parse_features() > > as convertor to global properties) > > > > On Thu, Jun 09, 2016 at 07:11:01PM +0200, Igor Mammedov wrote: > > [...] > > > -static void cpu_common_parse_features(CPUState *cpu, char *features, > > > +static void cpu_common_parse_features(const char *typename, char > > > *features, > > > Error **errp) > > > { > > > char *featurestr; /* Single "key=value" string being parsed */ > > > char *val; > > > - Error *err = NULL; > > > + static bool cpu_globals_initialized; > > > + > > > + /* TODO: all callers of ->parse_features() need to be changed to > > > + * call it only once, so we can remove this check (or change it > > > + * to assert(!cpu_globals_initialized). > > > + * Current callers of ->parse_features() are: > > > + * - machvirt_init() > > > + * - cpu_generic_init() > > > + * - cpu_x86_create() > > > + */ > > > + if (cpu_globals_initialized) { > > > + return; > > > + } > > > + cpu_globals_initialized = true; > > > > > > featurestr = features ? strtok(features, ",") : NULL; > > > > > > while (featurestr) { > > > val = strchr(featurestr, '='); > > > if (val) { > > > + GlobalProperty *prop = g_new0(typeof(*prop), 1); > > > *val = 0; > > > val++; > > > - object_property_parse(OBJECT(cpu), val, featurestr, &err); > > > - if (err) { > > > - error_propagate(errp, err); > > > - return; > > > - } > > > + prop->driver = typename; > > > + prop->property = g_strdup(featurestr); > > > + prop->value = g_strdup(val); > > > + qdev_prop_register_global(prop); > > > > This allows the user to trigger an assert: > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on > > qemu-system-x86_64: hw/core/qdev-properties.c:1087: > > qdev_prop_set_globals_for_type: Assertion `prop->user_provided' failed. > > Aborted (core dumped) > > > > but even if we fix the assert by setting > > prop->user_provided=true, we have a problem. Previous behavior > > was: > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on > > qemu-system-x86_64: Property '.INVALID' not found > > $ > > > > after this patch, and setting prop->user_provided=true, we have: > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on > > qemu-system-x86_64: Warning: global qemu64-x86_64-cpu.INVALID=on ignored: > > Property '.INVALID' not found > > [QEMU keeps running] > > > > QEMU needs to refuse to run if an invalid property is specified > > on -cpu. It is an important mechanism to prevent VMs from running > > if the user is requesting for a unsupported feature that requires > > newer QEMU. > > > > Any suggestions on how to fix that? > > Replace user_provided with a Error* argument to qdev_prop_register_global. > It can be &error_abort and NULL for the current users of the function, > while -cpu can use &error_fatal. Brilliant. This should work. -- Eduardo