diff mbox

[10/10] machine: Skip global registration for non-existing classes

Message ID 1466022773-8965-11-git-send-email-ehabkost@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eduardo Habkost June 15, 2016, 8:32 p.m. UTC
MachineClass::compat_props may point to class names that are not
compiled into the QEMU binary. Skip registering those as global
properties. This will allow the qdev global property code to
implement stricter checks on the global property values in the
future.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/machine.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Marcel Apfelbaum June 19, 2016, 4:39 p.m. UTC | #1
On 06/15/2016 11:32 PM, Eduardo Habkost wrote:
> MachineClass::compat_props may point to class names that are not
> compiled into the QEMU binary. Skip registering those as global
> properties. This will allow the qdev global property code to
> implement stricter checks on the global property values in the
> future.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/core/machine.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d6f6be4..51697cb 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -583,6 +583,7 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
>   void machine_register_compat_props(MachineState *machine)
>   {
>       MachineClass *mc = MACHINE_GET_CLASS(machine);
> +    ObjectClass *oc;
>       int i;
>       GlobalProperty *p;
>
> @@ -592,6 +593,14 @@ void machine_register_compat_props(MachineState *machine)
>
>       for (i = 0; i < mc->compat_props->len; i++) {
>           p = g_array_index(mc->compat_props, GlobalProperty *, i);

Hi Eduardo,

> +
> +        /* Skip registering globals for non-existing device classes */
> +        oc = object_class_by_name(p->driver);

This I understand. It ensure a corresponding class is in QEMU binary.
But even so, if such property is not available should we silently continue?
Maybe the user things the property is set...


> +        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);

This I don't understand. Do we check here?

Thanks,
Marcel

> +        if (!oc) {
> +            continue;
> +        }
> +
>           /* Machine compat_props must never cause errors: */
>           p->errp = &error_abort;
>           qdev_prop_register_global(p);
>
Marcel Apfelbaum June 19, 2016, 4:50 p.m. UTC | #2
On 06/19/2016 07:39 PM, Marcel Apfelbaum wrote:
> On 06/15/2016 11:32 PM, Eduardo Habkost wrote:
>> MachineClass::compat_props may point to class names that are not
>> compiled into the QEMU binary. Skip registering those as global
>> properties. This will allow the qdev global property code to
>> implement stricter checks on the global property values in the
>> future.
>>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>   hw/core/machine.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index d6f6be4..51697cb 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -583,6 +583,7 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
>>   void machine_register_compat_props(MachineState *machine)
>>   {
>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>> +    ObjectClass *oc;
>>       int i;
>>       GlobalProperty *p;
>>
>> @@ -592,6 +593,14 @@ void machine_register_compat_props(MachineState *machine)
>>
>>       for (i = 0; i < mc->compat_props->len; i++) {
>>           p = g_array_index(mc->compat_props, GlobalProperty *, i);
>
> Hi Eduardo,
>
>> +
>> +        /* Skip registering globals for non-existing device classes */
>> +        oc = object_class_by_name(p->driver);
>
> This I understand. It ensure a corresponding class is in QEMU binary.
> But even so, if such property is not available should we silently continue?
> Maybe the user things the property is set...

Please forgive the spelling errors and disregard the above question.

>
>
>> +        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
>
> This I don't understand. Do we check here?
>

Regarding my last question, it seems you followed an existing pattern
here, however I don't quite understand, we found the class,
why do we need to ensure is a TYPE_DEVICE?

What error case do we want to prevent?

Thanks,
Marcel

> Thanks,
> Marcel
>
>> +        if (!oc) {
>> +            continue;
>> +        }
>> +
>>           /* Machine compat_props must never cause errors: */
>>           p->errp = &error_abort;
>>           qdev_prop_register_global(p);
>>
>
Eduardo Habkost June 20, 2016, 1:38 p.m. UTC | #3
On Sun, Jun 19, 2016 at 07:50:56PM +0300, Marcel Apfelbaum wrote:
> On 06/19/2016 07:39 PM, Marcel Apfelbaum wrote:
> > On 06/15/2016 11:32 PM, Eduardo Habkost wrote:
> > > MachineClass::compat_props may point to class names that are not
> > > compiled into the QEMU binary. Skip registering those as global
> > > properties. This will allow the qdev global property code to
> > > implement stricter checks on the global property values in the
> > > future.
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >   hw/core/machine.c | 9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index d6f6be4..51697cb 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -583,6 +583,7 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
> > >   void machine_register_compat_props(MachineState *machine)
> > >   {
> > >       MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > +    ObjectClass *oc;
> > >       int i;
> > >       GlobalProperty *p;
> > > 
> > > @@ -592,6 +593,14 @@ void machine_register_compat_props(MachineState *machine)
> > > 
> > >       for (i = 0; i < mc->compat_props->len; i++) {
> > >           p = g_array_index(mc->compat_props, GlobalProperty *, i);
> > 
> > Hi Eduardo,
> > 
> > > +
> > > +        /* Skip registering globals for non-existing device classes */
> > > +        oc = object_class_by_name(p->driver);
> > 
> > This I understand. It ensure a corresponding class is in QEMU binary.
> > But even so, if such property is not available should we silently continue?
> > Maybe the user things the property is set...
> 
> Please forgive the spelling errors and disregard the above question.
> 
> > 
> > 
> > > +        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> > 
> > This I don't understand. Do we check here?
> > 
> 
> Regarding my last question, it seems you followed an existing pattern
> here, however I don't quite understand, we found the class,
> why do we need to ensure is a TYPE_DEVICE?
> 
> What error case do we want to prevent?

Globals are a TYPE_DEVICE feature (implemented in
device_post_init()). Registering globals for non-TYPE_DEVICE
classes don't work.
diff mbox

Patch

diff --git a/hw/core/machine.c b/hw/core/machine.c
index d6f6be4..51697cb 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -583,6 +583,7 @@  static void machine_class_finalize(ObjectClass *klass, void *data)
 void machine_register_compat_props(MachineState *machine)
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
+    ObjectClass *oc;
     int i;
     GlobalProperty *p;
 
@@ -592,6 +593,14 @@  void machine_register_compat_props(MachineState *machine)
 
     for (i = 0; i < mc->compat_props->len; i++) {
         p = g_array_index(mc->compat_props, GlobalProperty *, i);
+
+        /* Skip registering globals for non-existing device classes */
+        oc = object_class_by_name(p->driver);
+        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
+        if (!oc) {
+            continue;
+        }
+
         /* Machine compat_props must never cause errors: */
         p->errp = &error_abort;
         qdev_prop_register_global(p);