From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58206) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCuJ9-0007Ng-MO for qemu-devel@nongnu.org; Tue, 14 Jun 2016 15:47:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCuJ7-0007bj-Rf for qemu-devel@nongnu.org; Tue, 14 Jun 2016 15:47:30 -0400 Date: Tue, 14 Jun 2016 16:47:18 -0300 From: Eduardo Habkost Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1465492263-28472-5-git-send-email-imammedo@redhat.com> Subject: [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: Igor Mammedov Cc: qemu-devel@nongnu.org, peter.maydell@linaro.org, mark.cave-ayland@ilande.co.uk, blauwirbel@gmail.com, qemu-arm@nongnu.org, pbonzini@redhat.com, rth@twiddle.net, Markus Armbruster 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? 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