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

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

2016-06-14 Thread Paolo Bonzini



- Original Message -
> From: "Eduardo Habkost" 
> To: "Igor Mammedov" 
> Cc: qemu-devel@nongnu.org, "peter maydell" , "mark 
> cave-ayland"
> , blauwir...@gmail.com, qemu-...@nongnu.org, 
> pbonz...@redhat.com, r...@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
>

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

2016-06-14 Thread Eduardo Habkost

On Tue, Jun 14, 2016 at 05:41:03PM -0400, Paolo Bonzini wrote:
> 
> 
> - Original Message -
> > From: "Eduardo Habkost" 
> > To: "Igor Mammedov" 
> > Cc: qemu-devel@nongnu.org, "peter maydell" , 
> > "mark cave-ayland"
> > , blauwir...@gmail.com, qemu-...@nongnu.org, 
> > pbonz...@redhat.com, r...@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.

Brilliant. This should work.

-- 
Eduardo

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

2016-06-14 Thread 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

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information