Re: [Qemu-devel] [PATCH v2 2/3] qemu-help: Sort devices by logical functionality

[Michael S. Tsirkin]

On Mon, Jul 29, 2013 at 10:07:34AM +0300, Marcel Apfelbaum wrote:
> Categorize devices that appear as output to "-device ?" command
> by logical functionality. Sort the devices by logical categories
> before showing them to user.
> 
> The sort is done by functionality rather than alphabetical.
> 
> Signed-off-by: Marcel Apfelbaum <address@hidden>
> ---
> Changes from v1:
> Addressed Michael Tsirkin review:
> Used bitmap operations on categories
> Moved category names into the header file
> 
>  include/hw/qdev-core.h | 33 +++++++++++++++++++++++++++++++++
>  qdev-monitor.c         | 50 
> ++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 79 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index e8b89b1..80b06ac 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -18,6 +18,38 @@ enum {
>  #define DEVICE_CLASS(klass) OBJECT_CLASS_CHECK(DeviceClass, (klass), 
> TYPE_DEVICE)
>  #define DEVICE_GET_CLASS(obj) OBJECT_GET_CLASS(DeviceClass, (obj), 
> TYPE_DEVICE)
>  
> +typedef enum DeviceCategory {
> +    DEVICE_CATEGORY_ASSEMBLY,
> +    DEVICE_CATEGORY_MANAGEMENT,
> +    DEVICE_CATEGORY_STORAGE,
> +    DEVICE_CATEGORY_NETWORK,
> +    DEVICE_CATEGORY_INPUT,
> +    DEVICE_CATEGORY_DISPLAY,
> +    DEVICE_CATEGORY_SOUND,
> +    DEVICE_CATEGORY_MISC,
> +    DEVICE_CATEGORY_MAX
> +} DeviceCategory;
> +
> +static inline const char *qdev_category_get_name(DeviceCategory category)
> +{
> +    /* Category names corresponding to DeviceCategory values
> +     * The array elements must be in the same order as they
> +     * appear in DeviceCategory enum.
> +     */

I would simply do:
     static const char *category_names[DEVICE_CATEGORY_MAX] = {
         [DEVICE_CATEGORY_ASSEMBLY] = "Assembly",
         [DEVICE_CATEGORY_MANAGEMENT] = "Management",
         [DEVICE_CATEGORY_STORAGE] = "Storage",
         [DEVICE_CATEGORY_NETWORK] = "Network",
         [DEVICE_CATEGORY_INPUT] = "Input",
         [DEVICE_CATEGORY_DISPLAY] = "Display",
         [DEVICE_CATEGORY_SOUND] = "Sound",
         [DEVICE_CATEGORY_MISC] = "Misc",
     };

and drop the requirement to use same order.

> +    static const char *category_names[] = {
> +        "Assembly",
> +        "Management",
> +        "Storage",
> +        "Network",
> +        "Input",
> +        "Display",
> +        "Sound",
> +        "Misc",
> +    };


> +
> +    return category_names[category];
> +}
> +
>  typedef int (*qdev_initfn)(DeviceState *dev);
>  typedef int (*qdev_event)(DeviceState *dev);
>  typedef void (*qdev_resetfn)(DeviceState *dev);
> @@ -81,6 +113,7 @@ typedef struct DeviceClass {
>      ObjectClass parent_class;
>      /*< public >*/
>  
> +    DECLARE_BITMAP(categories, 20);

Why 20? Not DEVICE_CATEGORY_MAX ?


>      const char *fw_name;
>      const char *desc;
>      Property *props;
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index e54dbc2..c3a3550 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -75,14 +75,21 @@ static bool qdev_class_has_alias(DeviceClass *dc)
>      return (qdev_class_get_alias(dc) != NULL);
>  }
>  
> +typedef struct PrintDevInfoData {
> +    bool show_no_user;
> +    DeviceCategory category;
> +} PrintDevInfoData ;
> +
>  static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
>  {
>      DeviceClass *dc;
> -    bool *show_no_user = opaque;
> +    PrintDevInfoData *data = opaque;

So all callers of qdev_print_devinfo would have to be updated,
but you only updated one caller.
Lack of type checking here is nasty. It's required
by the object_class_foreach but you are not using that
anymore.

So please refactor this function: e.g.
qdev_print_devinfo which gets void *,
and qdev_print_class_devinfo which gets show_no_user and category.
In fact, this way there won't be need for use of void *
at all.


> +    DeviceCategory category;
>  
> +    category = data ? data->category : DEVICE_CATEGORY_MAX;
>      dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
>  
> -    if (!dc || (show_no_user && !*show_no_user && dc->no_user)) {
> +    if (!dc || (data && !data->show_no_user && dc->no_user)) {
>          return;
>      }
>  
> @@ -93,6 +100,18 @@ static void qdev_print_devinfo(ObjectClass *klass, void 
> *opaque)
>      if (qdev_class_has_alias(dc)) {
>          error_printf(", alias \"%s\"", qdev_class_get_alias(dc));
>      }
> +    if (dc->categories) {

Is this sometimes unset? Some devices don't have a category?

> +        if (test_bit(category, dc->categories)) {
> +            error_printf(", category \"%s\"", 
> qdev_category_get_name(category));
> +        } else {
> +            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));
> +                }
> +            }

Confused. Why different output format?
Maybe add a comment with explanation.
Also, testing DEVICE_CATEGORY_MAX will cause out of bounds
access on bitmap - you want to special-case it
explicitly.

Also, this overrides category parameter so if one tries to
use it below one gets DEVICE_CATEGORY_MAX.
Better use a different local variable for the loop.

> +        }
> +    }
>      if (dc->desc) {
>          error_printf(", desc \"%s\"", dc->desc);
>      }
> @@ -139,6 +158,23 @@ static const char *find_typename_by_alias(const char 
> *alias)
>      return NULL;
>  }
>  
> +static GSList *qdev_get_devices_by_category(DeviceCategory category)
> +{
> +    DeviceClass *dc;
> +    GSList *list, *curr, *ret_list = NULL;
> +
> +    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 (test_bit(category, dc->categories)) {
> +            ret_list = g_slist_append(ret_list, dc);

So you build list here, then immediately scan it in
the same order and free it.
Wouldn't it be easier to just call qdev_print_devinfo here
directly?


> +        }
> +    }
> +    g_slist_free(list);
> +
> +    return ret_list;
> +}
> +
>  int qdev_device_help(QemuOpts *opts)
>  {
>      const char *driver;
> @@ -147,8 +183,14 @@ int qdev_device_help(QemuOpts *opts)
>  
>      driver = qemu_opt_get(opts, "driver");
>      if (driver && is_help_option(driver)) {
> -        bool show_no_user = false;
> -        object_class_foreach(qdev_print_devinfo, TYPE_DEVICE, false, 
> &show_no_user);
> +        DeviceCategory category;
> +        for (category = 0; category < DEVICE_CATEGORY_MAX; ++category) {
> +            PrintDevInfoData data = { false, category };
> +            GSList *list = qdev_get_devices_by_category(category);
> +            g_slist_foreach(list, (GFunc)qdev_print_devinfo, &data);

This cast should be avoided. If function signatures do not match,
you really don't want to work around it like this.
Refactor code please.


> +            g_slist_free(list);
> +        }
> +
>          return 1;
>      }
>  
> -- 
> 1.8.3.1