[08/16] qom: introduce object_register_sugar_prop

Similar to the existing "-rtc driftfix" option, we will convert some
legacy "-machine" command line options to global properties on accelerators.
Because accelerators are not devices, we cannot use qdev_prop_register_global.
Instead, provide a slot in the generic object_compat_props arrays for
command line syntactic sugar.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qom/object.h |  1 +
 qom/object.c         | 23 +++++++++++++++++++++--
 vl.c                 | 10 +++-------
 3 files changed, 25 insertions(+), 9 deletions(-)

Hi

On Wed, Nov 13, 2019 at 6:46 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Similar to the existing "-rtc driftfix" option, we will convert some
> legacy "-machine" command line options to global properties on accelerators.
> Because accelerators are not devices, we cannot use qdev_prop_register_global.
> Instead, provide a slot in the generic object_compat_props arrays for
> command line syntactic sugar.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

sounds reasonable

> ---
>  include/qom/object.h |  1 +
>  qom/object.c         | 23 +++++++++++++++++++++--
>  vl.c                 | 10 +++-------
>  3 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 128d00c..230b18f 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -679,6 +679,7 @@ void object_apply_global_props(Object *obj, const GPtrArray *props,
>                                 Error **errp);
>  void object_set_machine_compat_props(GPtrArray *compat_props);
>  void object_set_accelerator_compat_props(GPtrArray *compat_props);
> +void object_register_sugar_prop(const char *driver, const char *prop, const char *value);

Or simply

void object_add_global_prop(const char *typename, ...) ?


>  void object_apply_compat_props(Object *obj);
>
>  /**
> diff --git a/qom/object.c b/qom/object.c
> index 6fa9c61..c7825dd 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -414,10 +414,29 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
>   * Global property defaults
>   * Slot 0: accelerator's global property defaults
>   * Slot 1: machine's global property defaults
> + * Slot 2: global properties from legacy command line option
>   * Each is a GPtrArray of of GlobalProperty.
>   * Applied in order, later entries override earlier ones.
>   */
> -static GPtrArray *object_compat_props[2];
> +static GPtrArray *object_compat_props[3];
> +
> +/*
> + * Retrieve @GPtrArray for global property defined with options
> + * other than "-global".  These are generally used for syntactic
> + * sugar and legacy command line options.
> + */
> +void object_register_sugar_prop(const char *driver, const char *prop, const char *value)
> +{
> +    GlobalProperty *g;
> +    if (!object_compat_props[2]) {
> +        object_compat_props[2] = g_ptr_array_new();
> +    }
> +    g = g_new(GlobalProperty, 1);
> +    g->driver = g_strdup(driver);
> +    g->property = g_strdup(prop);
> +    g->value = g_strdup(value);
> +    g_ptr_array_add(object_compat_props[2], g);
> +}
>
>  /*
>   * Set machine's global property defaults to @compat_props.
> @@ -445,7 +464,7 @@ void object_apply_compat_props(Object *obj)
>
>      for (i = 0; i < ARRAY_SIZE(object_compat_props); i++) {
>          object_apply_global_props(obj, object_compat_props[i],
> -                                  &error_abort);
> +                                  i == 2 ? &error_fatal : &error_abort);

Isn't error_abort() appropriate in all cases?


>      }
>  }
>
> diff --git a/vl.c b/vl.c
> index 843b263..cb993dd 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -896,13 +896,9 @@ static void configure_rtc(QemuOpts *opts)
>      value = qemu_opt_get(opts, "driftfix");
>      if (value) {
>          if (!strcmp(value, "slew")) {
> -            static GlobalProperty slew_lost_ticks = {
> -                .driver   = "mc146818rtc",
> -                .property = "lost_tick_policy",
> -                .value    = "slew",
> -            };
> -
> -            qdev_prop_register_global(&slew_lost_ticks);
> +            object_register_sugar_prop("mc146818rtc",
> +                                       "lost_tick_policy",
> +                                       "slew");

Why do you convert this since it's a device?

>          } else if (!strcmp(value, "none")) {
>              /* discard is default */
>          } else {
> --
> 1.8.3.1
>
>
>

On 14/11/19 10:53, Marc-André Lureau wrote:
>>  include/qom/object.h |  1 +
>>  qom/object.c         | 23 +++++++++++++++++++++--
>>  vl.c                 | 10 +++-------
>>  3 files changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 128d00c..230b18f 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -679,6 +679,7 @@ void object_apply_global_props(Object *obj, const GPtrArray *props,
>>                                 Error **errp);
>>  void object_set_machine_compat_props(GPtrArray *compat_props);
>>  void object_set_accelerator_compat_props(GPtrArray *compat_props);
>> +void object_register_sugar_prop(const char *driver, const char *prop, const char *value);
> 
> Or simply
> 
> void object_add_global_prop(const char *typename, ...) ?

This is actually how I called it first, but I didn't like it because
it's prioritized _below_ -global, and it's easy to confuse it with
object_add_global_prop.

>>  /*
>>   * Set machine's global property defaults to @compat_props.
>> @@ -445,7 +464,7 @@ void object_apply_compat_props(Object *obj)
>>
>>      for (i = 0; i < ARRAY_SIZE(object_compat_props); i++) {
>>          object_apply_global_props(obj, object_compat_props[i],
>> -                                  &error_abort);
>> +                                  i == 2 ? &error_fatal : &error_abort);
> 
> Isn't error_abort() appropriate in all cases?

Unfortunately not, because otherwise "-accel tcg,tb-size=foo" would crash.

>>      }
>>  }
>>
>> diff --git a/vl.c b/vl.c
>> index 843b263..cb993dd 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -896,13 +896,9 @@ static void configure_rtc(QemuOpts *opts)
>>      value = qemu_opt_get(opts, "driftfix");
>>      if (value) {
>>          if (!strcmp(value, "slew")) {
>> -            static GlobalProperty slew_lost_ticks = {
>> -                .driver   = "mc146818rtc",
>> -                .property = "lost_tick_policy",
>> -                .value    = "slew",
>> -            };
>> -
>> -            qdev_prop_register_global(&slew_lost_ticks);
>> +            object_register_sugar_prop("mc146818rtc",
>> +                                       "lost_tick_policy",
>> +                                       "slew");
> 
> Why do you convert this since it's a device?

Not strictly necessary, but it's more compact and it more or less
matches the usecase for this function.

Paolo