| Message ID | 1375081655-28541-3-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <marcel.a@redhat.com> > --- > 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
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 <marcel.a@redhat.com> > > --- > > 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 > > > + 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 > 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
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 <marcel.a@redhat.com> > > > --- > > > 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. > > > > > + 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? 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. > > 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 >
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 <marcel.a@redhat.com> > > > > --- > > > > 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. Also it will not be easy to differentiate devices belonging to the same category without grep. > 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 > >
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 <marcel.a@redhat.com> > > > > > --- > > > > > 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 > > > >
On Mon, 2013-07-29 at 12:22 +0300, Michael S. Tsirkin wrote: > 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 <marcel.a@redhat.com> > > > > > > --- > > > > > > 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? When there are headers I as suggested bellow - yes, otherwise annoying > > > 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. With the headers - sure Before them it is not as helpful. We don't have multifunction yet, but when we will have such devices, the headers will be very handy In the meantime we can go with printing all the categories > > > > 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 > > > > > >
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. + */ + 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); 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; + 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) { + 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)); + } + } + } + } 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); + } + } + 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); + g_slist_free(list); + } + 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 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(-)