Re: [Qemu-devel] [PATCH 1/2] Mostly revert "qemu-help: Sort devices by logical functionality"

[Marcel Apfelbaum]

On Thu, 2013-10-10 at 15:00 +0200, address@hidden wrote:
> From: Markus Armbruster <address@hidden>
> 
> This reverts most of commit 3d1237fb2ab4edb926c717767bb5e31d6053a7c5.
> 
> The commit claims to sort the output of "-device help" "by
> functionality rather than alphabetical".  Issues:
> 
> * The output was unsorted before, not alphabetically sorted.
>   Misleading, but harmless enough.
I meant that will be sorted, but the sorting will be by functionality
rather than alphabetical. I can understand the confusion.

> 
> * The commit doesn't just sort the output of "-device help" as it
>   claims, it adds categories to each line of "-device help", and it
>   prints devices once per category.  In particular, devices without a
>   category aren't shown anymore.  Maybe such devices should not exist,
>   but they do.  Regression.
Yes - :(, it was a drawback that devices with no category are not
printed. On the other hand all devices must be attached to a category,
otherwise we will have again a lot of devices with no category.

I suppose your approach lets us at least with the possibility to see
these devices giving us a chance to attach them to their category.

> 
> * Categories are also added to the output of "info qdm".  Silent
>   change, not nice.  Output remains unsorted, unlike "-device help".

I checked libvirt and the parsing of the output is not affected
by adding the category. 

I still think that adding the category in each line may be
useful when using grep, but I suppose that we can grep by category 
with -A x.

Thanks,
Marcel 

> I'm going to reimplement the feature we actually want, without the
> warts.  Reverting the flawed commit first should make it easier to
> review.  However, I can't revert it completely, since DeviceClass
> member categories has been put to use.  So leave that part in.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  include/hw/qdev-core.h | 16 ----------------
>  qdev-monitor.c         | 48 +++++++++---------------------------------------
>  2 files changed, 9 insertions(+), 55 deletions(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index a62f231..e191ca0 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -30,22 +30,6 @@ typedef enum DeviceCategory {
>      DEVICE_CATEGORY_MAX
>  } DeviceCategory;
>  
> -static inline const char *qdev_category_get_name(DeviceCategory category)
> -{
> -    static const char *category_names[DEVICE_CATEGORY_MAX] = {
> -        [DEVICE_CATEGORY_BRIDGE]  = "Controller/Bridge/Hub",
> -        [DEVICE_CATEGORY_USB]     = "USB",
> -        [DEVICE_CATEGORY_STORAGE] = "Storage",
> -        [DEVICE_CATEGORY_NETWORK] = "Network",
> -        [DEVICE_CATEGORY_INPUT]   = "Input",
> -        [DEVICE_CATEGORY_DISPLAY] = "Display",
> -        [DEVICE_CATEGORY_SOUND]   = "Sound",
> -        [DEVICE_CATEGORY_MISC]    = "Misc",
> -    };
> -
> -    return category_names[category];
> -};
> -
>  typedef int (*qdev_initfn)(DeviceState *dev);
>  typedef int (*qdev_event)(DeviceState *dev);
>  typedef void (*qdev_resetfn)(DeviceState *dev);
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 410cdcb..e5adf6c 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -75,27 +75,24 @@ static bool qdev_class_has_alias(DeviceClass *dc)
>      return (qdev_class_get_alias(dc) != NULL);
>  }
>  
> -static void qdev_print_class_devinfo(DeviceClass *dc)
> +static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
>  {
> -    DeviceCategory category;
> +    DeviceClass *dc;
> +    bool *show_no_user = opaque;
> +
> +    dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
>  
> -    if (!dc) {
> +    if (!dc || (show_no_user && !*show_no_user && dc->no_user)) {
>          return;
>      }
>  
> -    error_printf("name \"%s\"", object_class_get_name(OBJECT_CLASS(dc)));
> +    error_printf("name \"%s\"", object_class_get_name(klass));
>      if (dc->bus_type) {
>          error_printf(", bus %s", dc->bus_type);
>      }
>      if (qdev_class_has_alias(dc)) {
>          error_printf(", alias \"%s\"", qdev_class_get_alias(dc));
>      }
> -    error_printf(", categories");
> -    for (category = 0; category < DEVICE_CATEGORY_MAX; ++category) {
> -        if (test_bit(category, dc->categories)) {
> -            error_printf(" \"%s\"", qdev_category_get_name(category));
> -        }
> -    }
>      if (dc->desc) {
>          error_printf(", desc \"%s\"", dc->desc);
>      }
> @@ -105,15 +102,6 @@ static void qdev_print_class_devinfo(DeviceClass *dc)
>      error_printf("\n");
>  }
>  
> -static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
> -{
> -    DeviceClass *dc;
> -
> -    dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
> -
> -    qdev_print_class_devinfo(dc);
> -}
> -
>  static int set_property(const char *name, const char *value, void *opaque)
>  {
>      DeviceState *dev = opaque;
> @@ -151,21 +139,6 @@ static const char *find_typename_by_alias(const char 
> *alias)
>      return NULL;
>  }
>  
> -static void qdev_print_category_devices(DeviceCategory category)
> -{
> -    DeviceClass *dc;
> -    GSList *list, *curr;
> -
> -    list = object_class_get_list(TYPE_DEVICE, false);
> -    for (curr = list; curr; curr = g_slist_next(curr)) {
> -        dc = (DeviceClass *)object_class_dynamic_cast(curr->data, 
> TYPE_DEVICE);
> -        if (!dc->no_user && test_bit(category, dc->categories)) {
> -            qdev_print_class_devinfo(dc);
> -        }
> -    }
> -    g_slist_free(list);
> -}
> -
>  int qdev_device_help(QemuOpts *opts)
>  {
>      const char *driver;
> @@ -174,11 +147,8 @@ int qdev_device_help(QemuOpts *opts)
>  
>      driver = qemu_opt_get(opts, "driver");
>      if (driver && is_help_option(driver)) {
> -        DeviceCategory category;
> -        for (category = 0; category < DEVICE_CATEGORY_MAX; ++category) {
> -            qdev_print_category_devices(category);
> -        }
> -
> +        bool show_no_user = false;
> +        object_class_foreach(qdev_print_devinfo, TYPE_DEVICE, false, 
> &show_no_user);
>          return 1;
>      }
>