From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33158) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCw5N-0000Dp-0P for qemu-devel@nongnu.org; Tue, 14 Jun 2016 17:41:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCw5K-00013L-U6 for qemu-devel@nongnu.org; Tue, 14 Jun 2016 17:41:24 -0400 Date: Tue, 14 Jun 2016 17:41:03 -0400 (EDT) From: Paolo Bonzini Message-ID: <1646409152.22421731.1465940463160.JavaMail.zimbra@redhat.com> In-Reply-To: <20160614194718.GJ17952@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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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: Eduardo Habkost Cc: Igor Mammedov , qemu-devel@nongnu.org, peter maydell , mark cave-ayland , blauwirbel@gmail.com, qemu-arm@nongnu.org, rth@twiddle.net, Markus Armbruster ----- 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. Paolo > Maybe qdev_prop_set_globals() can collect errors in a list in > DeviceState, and we can check for them in code that creates > device objects (like cpu_generic_init(), qdev_device_add()), or > in the beginning of device_set_realized(). > > -- > Eduardo >