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

[Eduardo Habkost]

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