qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] Handling errors caused by -global (was Re: [PATCH v2 4/


From: Paolo Bonzini
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)
Date: Tue, 14 Jun 2016 17:41:03 -0400 (EDT)


----- Original Message -----
> From: "Eduardo Habkost" <address@hidden>
> To: "Igor Mammedov" <address@hidden>
> Cc: address@hidden, "peter maydell" <address@hidden>, "mark cave-ayland"
> <address@hidden>, address@hidden, address@hidden, address@hidden, 
> address@hidden,
> "Markus Armbruster" <address@hidden>
> 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
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]