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

[Michael S. Tsirkin]

On Mon, Jul 29, 2013 at 12:09:45PM +0300, Marcel Apfelbaum wrote:
> On Mon, 2013-07-29 at 11:20 +0300, Michael S. Tsirkin wrote:
> > On Mon, Jul 29, 2013 at 11:14:11AM +0300, Marcel Apfelbaum wrote:
> > > On Mon, 2013-07-29 at 11:04 +0300, Michael S. Tsirkin wrote:
> > > > 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.
> > > OK
> > > > 
> > > > > +    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 ?
> > > OK
> > > > 
> > > > 
> > > > >      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.
> > > OK
> > > > 
> > > > 
> > > > > +    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?
> > > All devices shall have a category
> > 
> > There's no point to the if statement above, apparently.
> > if (dc->categories) actually tests the array pointer.
> > Since that's defined statically using DECLARE_BITMAP, it's
> > never NULL.
> Right
> > 
> > 
> > > > 
> > > > > +        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.
> > > > 
> > > In RFC patches was possible to print a device that belonged
> > > to a specific category, or print out all its categories.
> > > In this series it can be done by passing DEVICE_CATEGORY_MAX
> > 
> > Yes but when you are printing a specific device, why not
> > always print all categories?
> Because multifunction devices will appear more than once
> in a grep.

Right. That's fine, isn't it?

> Also it will not be easy to differentiate devices
> belonging to the same category without grep.

Hmm not sure what you mean.
I expect long term we'll add headers like

Network devices:
        a
        b
        c
Storage devices
        a
        e
        f

and it seems more helpful to tell user that a is
both a network and a storage device,
than just rely on user noticing that it appears twice.

> > And assuming you don't want to print all categories,
> > do if(category == DEVICE_CATEGORY_MAX), test_bit does not
> > check that it's not a legal value.
> This is exactly what I am going to do.
> > 
> > > > 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.
> > > OK
> > > > 
> > > > > +        }
> > > > > +    }
> > > > >      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.
> > > OK
> > > > 
> > > > 
> > > > > +            g_slist_free(list);
> > > > > +        }
> > > > > +
> > > > >          return 1;
> > > > >      }
> > > > >  
> > > > > -- 
> > > > > 1.8.3.1
> > > 
>