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

From: Markus Armbruster <armbru@redhat.com>

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.

* 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.

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

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 <armbru@redhat.com>
---
 include/hw/qdev-core.h | 16 ----------------
 qdev-monitor.c         | 48 +++++++++---------------------------------------
 2 files changed, 9 insertions(+), 55 deletions(-)

On Thu, 2013-10-10 at 15:00 +0200, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> 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 <armbru@redhat.com>
> ---
>  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;
>      }
>

Marcel Apfelbaum <marcel.a@redhat.com> writes:

> On Thu, 2013-10-10 at 15:00 +0200, armbru@redhat.com wrote:
>> From: Markus Armbruster <armbru@redhat.com>
>> 
>> 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.

Happens :)

>> * 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.

s/otherweise will have again/but unfortunately we still have/

> 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.

Yes.

If you want to enforce "no uncategorized devices", you need to catch
them reliably at compile time or "make check" time.  Once that's done,
we can simplify my qdev_print_class_devinfos() not to look for such
devices.  Patches welcome :)

>> * 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 didn't claim actual harm, just pointed out that you should've
mentioned it in the commit message, and that the reasons for improving
"device_add help" equally apply to "info qdm".

> 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.

Yes, repeating the categories on each line can be convenient when the
lines are fed to programs, but help output is primarily for humans.

Try 'sed -n "/^$cat devices:/,/^\$/p"' instead of 'grep $cat'.

[...]