Re: [Qemu-devel] [PATCH v2 01/21] qdev: Replace cannot_instantiate_with_device_add_yet with !user_creatable

[Thomas Huth]

On 04.04.2017 22:24, Eduardo Habkost wrote:
> cannot_instantiate_with_device_add_yet was introduced by commit
> 837d37167dc446af8a91189108b363c04609e296 to replace no_user. It
> was supposed to be a temporary measure.

I think you rather meant commit efec3dd631d94160288392721a5f9c39e50fb2bc
("qdev: Replace no_user by cannot_instantiate_with_device_add_yet") ?

> When it was introduced, we had 54
> cannot_instantiate_with_device_add_yet=true lines in the code.
> Today (3 years later) this number has not shrinked: we now have
> 57 cannot_instantiate_with_device_add_yet=true lines. I think it
> is safe to say it is not a temporary measure, and we won't see
> the flag go away soon.

I fully agree.

> Instead of a long field name that misleads people to believe it
> is temporary, replace it a shorter and less misleading field:
> user_creatable.

Sounds much better, indeed. Thanks for tackling this!

[...]
>  static void rtc_finalize(Object *obj)
> diff --git a/monitor.c b/monitor.c
> index be282ecb80..e06edec2bd 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3156,7 +3156,7 @@ void device_add_completion(ReadLineState *rs, int 
> nb_args, const char *str)
>                                               TYPE_DEVICE);
>          name = object_class_get_name(OBJECT_CLASS(dc));
>  
> -        if (!dc->cannot_instantiate_with_device_add_yet
> +        if (dc->user_creatable
>              && !strncmp(name, str, len)) {

Cosmetics: You could put the strcmp now into the previous line, too.

>              readline_add_completion(rs, name);
>          }
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 5f2fcdfc45..e4c180c6bc 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -113,7 +113,7 @@ static void qdev_print_devinfo(DeviceClass *dc)
>      if (dc->desc) {
>          error_printf(", desc \"%s\"", dc->desc);
>      }
> -    if (dc->cannot_instantiate_with_device_add_yet) {
> +    if (!dc->user_creatable) {
>          error_printf(", no-user");
>      }
>      error_printf("\n");
> @@ -155,7 +155,7 @@ static void qdev_print_devinfos(bool show_no_user)
>                   ? !test_bit(i, dc->categories)
>                   : !bitmap_empty(dc->categories, DEVICE_CATEGORY_MAX))
>                  || (!show_no_user
> -                    && dc->cannot_instantiate_with_device_add_yet)) {
> +                    && !dc->user_creatable)) {

Cosmetics: That line could now also be moved at the end of the previous
line.

>                  continue;
>              }
>              if (!cat_printed) {
> @@ -240,7 +240,7 @@ static DeviceClass *qdev_get_device_class(const char 
> **driver, Error **errp)
>      }
>  
>      dc = DEVICE_CLASS(oc);
> -    if (dc->cannot_instantiate_with_device_add_yet ||
> +    if (!dc->user_creatable ||
>          (qdev_hotplug && !dc->hotpluggable)) {

dito

Anyway, patch looks fine to me, so:

Reviewed-by: Thomas Huth <address@hidden>