On Mon, 2013-07-29 at 15:42 +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 29, 2013 at 03:36:24PM +0300, Marcel Apfelbaum wrote:
> > On Mon, 2013-07-29 at 15:28 +0300, Michael S. Tsirkin wrote:
> > > 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 <marce...@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.
> > Yes, I saw that.... , but
> > - maybe the cleaning shall be done by a separate patch and not in this
> > series.
>
> I don't see how. Before your patch there are two callers so
> show_no_user is not always NULL.
>
> > - maybe someone will use this flag?
>
> It's a static function, not part of interface.
I'll include this change int the next version
>
> > >
> > > > + qdev_print_class_devinfo(dc);
>
> Wait a second, if device has no_user flag set,
> qdev_print_class_devinfo will not print anything,
> this is a bug: we want to be able to see
> properties of builtin devices.
I will remove the check.
Anyway it seems that in the new series show_no_user
checks will disappear
>
> > > > +}
> > > > +
> > > > 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.
> > qdev_print_class_devinfo() handles this check
>
>
> That's a bug, qdev_print_class_devinfo is called from
> qdev_print_devinfo which should be able to dump info
> about an abstract class.
>
> > >
> > > > + 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
> >
