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

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jul 29, 2013 at 03:11:42PM +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 <marcel.a@redhat.com>
> ---
> Changes from v2:
>     Addressed Michael Tsirkin's review:
>     - Explicit connection between the categories
>       and their names
>     - Refactoring of unsafe code
>     Addressed Paolo Bonzini's review
>     - Replaced Management category by USB 
> 
> Changes from v1:
>     Addressed Michael Tsirkin's review:
>     - Used bitmap operations on categories
>     - Moved category names into the header file
> 
> include/hw/qdev-core.h | 29 ++++++++++++++++++++++++++++
>  qdev-monitor.c         | 52 +++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 72 insertions(+), 9 deletions(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index e8b89b1..111ad06 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -18,6 +18,34 @@ 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_USB,
> +    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)
> +{
> +    static const char *category_names[DEVICE_CATEGORY_MAX] = {
> +        [DEVICE_CATEGORY_ASSEMBLY] = "Assembly",
> +        [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);
> @@ -81,6 +109,7 @@ typedef struct DeviceClass {
>      ObjectClass parent_class;
>      /*< public >*/
>  
> +    DECLARE_BITMAP(categories, DEVICE_CATEGORY_MAX);
>      const char *fw_name;
>      const char *desc;
>      Property *props;
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index e54dbc2..536e246 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -75,24 +75,27 @@ static bool qdev_class_has_alias(DeviceClass *dc)
>      return (qdev_class_get_alias(dc) != NULL);
>  }
>  
> -static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
> +static void qdev_print_class_devinfo(DeviceClass *dc)
>  {
> -    DeviceClass *dc;
> -    bool *show_no_user = opaque;
> -
> -    dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
> +    DeviceCategory category;
>  
> -    if (!dc || (show_no_user && !*show_no_user && dc->no_user)) {
> +    if (!dc || dc->no_user) {
>          return;
>      }
>  
> -    error_printf("name \"%s\"", object_class_get_name(klass));
> +    error_printf("name \"%s\"", object_class_get_name(OBJECT_CLASS(dc)));
>      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);
>      }
> @@ -102,6 +105,20 @@ static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
>      error_printf("\n");
>  }
>  
> +static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
> +{
> +    DeviceClass *dc;
> +    bool *show_no_user = opaque;
> +
> +    dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
> +
> +    if (!dc || (show_no_user && !*show_no_user && dc->no_user)) {
> +        return;
> +    }
> +

Looks like the only user of this function is passing in NULL opaque now,
so this is equivalent to if (!dc)? The rest is dead code?
I'm not sure dc is ever NULL BTW, but maybe for some devices it is.

> +    qdev_print_class_devinfo(dc);
> +}
> +
>  static int set_property(const char *name, const char *value, void *opaque)
>  {
>      DeviceState *dev = opaque;
> @@ -139,6 +156,20 @@ 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);

So here don't we need if (dc->no_user)) { continue; } ?
Otherwise we'll get a list of internal devices which
can not be instanciated with -device.

> +        if (test_bit(category, dc->categories)) {
> +            qdev_print_class_devinfo(dc);
> +        }
> +    }
> +}
> +
>  int qdev_device_help(QemuOpts *opts)
>  {
>      const char *driver;
> @@ -147,8 +178,11 @@ 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) {
> +            qdev_print_category_devices(category);
> +        }
> +
>          return 1;
>      }
>  
> -- 
> 1.8.3.1