| Message ID | 1375099903-17335-3-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
On Mon, 2013-07-29 at 15:11 +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; > + } > + > + 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); > + if (test_bit(category, dc->categories)) { > + qdev_print_class_devinfo(dc); > + } > + } Here is a memory leak. It will be fixed in v4 Marcel > +} > + > 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; > } >
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 <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. Yes, I saw that.... , but - maybe the cleaning shall be done by a separate patch and not in this series. - maybe someone will use this flag? > > > + 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. qdev_print_class_devinfo() handles this check > > > + 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
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 <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. > 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. > > > > > + 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. > > > +} > > > + > > > 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 >
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 <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. > > 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 > >
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; + } + + 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); + 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; }
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(-)