Re: [Qemu-devel] Handling errors caused by -global (was Re: [PATCH v2 4/6] cpu: use CPUClass->parse_features() as convertor to global properties)

[Paolo Bonzini <pbonzini@redhat.com>]

----- Original Message -----
> From: "Eduardo Habkost" <ehabkost@redhat.com>
> To: "Igor Mammedov" <imammedo@redhat.com>
> Cc: qemu-devel@nongnu.org, "peter maydell" <peter.maydell@linaro.org>, "mark cave-ayland"
> <mark.cave-ayland@ilande.co.uk>, blauwirbel@gmail.com, qemu-arm@nongnu.org, pbonzini@redhat.com, rth@twiddle.net,
> "Markus Armbruster" <armbru@redhat.com>
> 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
>