Message ID | 1375002894-26742-2-git-send-email-marcel.a@redhat.com |
---|---|
State | New |
Headers | show |
On Sun, Jul 28, 2013 at 12:14:53PM +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 RFC > Made category a bitmap to support multifunction PCI devices > > include/hw/qdev-core.h | 15 ++++++++++++ > qdev-monitor.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 75 insertions(+), 2 deletions(-) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 7fbffcb..9130f97 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -17,6 +17,19 @@ enum { > #define DEVICE_CLASS(klass) OBJECT_CLASS_CHECK(DeviceClass, (klass), TYPE_DEVICE) > #define DEVICE_GET_CLASS(obj) OBJECT_GET_CLASS(DeviceClass, (obj), TYPE_DEVICE) > > +#define MAX_DEVICE_CATEGORY 8 make this the last entry in the enum, also rename DEVICE_CATEGORY_MAX. This way you won't have to change it manually each time you add a category. > + > +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, > +} DeviceCategory; > + I think it's nasty that all users need to shift. One option is to name the enum fields with _SHIFT or _BIT suffix, and define macros #define DEVICE_CATEGORY_ASSEMBLY (1 << DEVICE_CATEGORY_ASSEMBLY_SHIFT) Then we don't need to have this shift everywhere these are used. Another is to use set_bit to set bits. I think I prefer this last variant. > typedef int (*qdev_initfn)(DeviceState *dev); > typedef int (*qdev_event)(DeviceState *dev); > typedef void (*qdev_resetfn)(DeviceState *dev); > @@ -80,6 +93,8 @@ typedef struct DeviceClass { > ObjectClass parent_class; > /*< public >*/ > > + /* DeviceCategory bitmap */ > + unsigned int categories; So why not use DECLARE_BITMAP? You will also be able to use test_bit instead of shifting everywhere. > const char *fw_name; > const char *desc; > Property *props; > diff --git a/qdev-monitor.c b/qdev-monitor.c > index e54dbc2..2e6d14e 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -51,6 +51,18 @@ static const QDevAlias qdev_alias_table[] = { > { } > }; > > +/* Category names corresponding to DeviceCategory values */ > +static const char *qdev_category_names[] = { > + "Assembly", > + "Management", > + "Storage", > + "Network", > + "Input", > + "Display", > + "Sound", > + "Misc", > +}; Offsets here correspond to specific enums. Maybe make this explicit? > +static const char *qdev_category_names[] = { > + .[DEVICE_CATEGORY_ASSEMBLY] = "Assembly", etc. Also, when adding a new category we'll need to add a new entry here. Maybe put both arrays together in the header so it's harder to miss, and add a comment about it? > + > static const char *qdev_class_get_alias(DeviceClass *dc) > { > const char *typename = object_class_get_name(OBJECT_CLASS(dc)); > @@ -75,11 +87,20 @@ 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 ; > + Why pointer to bool/category? Cleaner to pass it by instance I think. > static void qdev_print_devinfo(ObjectClass *klass, void *opaque) > { > DeviceClass *dc; > - bool *show_no_user = opaque; > + PrintDevInfoData *data = opaque; > + bool *show_no_user; > + DeviceCategory category; > > + show_no_user = data ? data->show_no_user : NULL; > + category = data && data->category ? *data->category : 0; > dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE); > > if (!dc || (show_no_user && !*show_no_user && dc->no_user)) { > @@ -93,6 +114,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 (dc->categories & (1 << category)) { > + error_printf(", category \"%s\"", qdev_category_names[category]); > + } else { > + error_printf(", categories"); > + for (category = 0; category < MAX_DEVICE_CATEGORY; ++category) { > + if (dc->categories & (1 << category)) { > + error_printf(" \"%s\"", qdev_category_names[category]); > + } > + } > + } > + } > if (dc->desc) { > error_printf(", desc \"%s\"", dc->desc); > } > @@ -139,6 +172,23 @@ static const char *find_typename_by_alias(const char *alias) > return NULL; > } > > +static GSList *qdev_get_devices_by_category(unsigned long categories) In practice this gets a single category right? So just pass the bit number, makes it more consistent. > +{ > + 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 ((dc->categories & categories) == categories) { I don't see where you would want to print only devices that match a mask exactly. With a single category, if (dc->categories & categories) is equivalent and easier to understand. > + ret_list = g_slist_append(ret_list, dc); > + } > + } > + g_slist_free(list); > + > + return ret_list; > +} > + > int qdev_device_help(QemuOpts *opts) > { > const char *driver; > @@ -148,7 +198,15 @@ 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; > + PrintDevInfoData data = { &show_no_user, &category }; > + So just drop show_no_user and category local vars, and use data.show_no_user data.category. > + for (category = 0; category < MAX_DEVICE_CATEGORY; ++category) { > + GSList *list = qdev_get_devices_by_category(1 << category); > + g_slist_foreach(list, (GFunc)qdev_print_devinfo, &data); > + 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 7fbffcb..9130f97 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -17,6 +17,19 @@ enum { #define DEVICE_CLASS(klass) OBJECT_CLASS_CHECK(DeviceClass, (klass), TYPE_DEVICE) #define DEVICE_GET_CLASS(obj) OBJECT_GET_CLASS(DeviceClass, (obj), TYPE_DEVICE) +#define MAX_DEVICE_CATEGORY 8 + +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, +} DeviceCategory; + typedef int (*qdev_initfn)(DeviceState *dev); typedef int (*qdev_event)(DeviceState *dev); typedef void (*qdev_resetfn)(DeviceState *dev); @@ -80,6 +93,8 @@ typedef struct DeviceClass { ObjectClass parent_class; /*< public >*/ + /* DeviceCategory bitmap */ + unsigned int categories; const char *fw_name; const char *desc; Property *props; diff --git a/qdev-monitor.c b/qdev-monitor.c index e54dbc2..2e6d14e 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -51,6 +51,18 @@ static const QDevAlias qdev_alias_table[] = { { } }; +/* Category names corresponding to DeviceCategory values */ +static const char *qdev_category_names[] = { + "Assembly", + "Management", + "Storage", + "Network", + "Input", + "Display", + "Sound", + "Misc", +}; + static const char *qdev_class_get_alias(DeviceClass *dc) { const char *typename = object_class_get_name(OBJECT_CLASS(dc)); @@ -75,11 +87,20 @@ 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; + bool *show_no_user; + DeviceCategory category; + show_no_user = data ? data->show_no_user : NULL; + category = data && data->category ? *data->category : 0; dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE); if (!dc || (show_no_user && !*show_no_user && dc->no_user)) { @@ -93,6 +114,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 (dc->categories & (1 << category)) { + error_printf(", category \"%s\"", qdev_category_names[category]); + } else { + error_printf(", categories"); + for (category = 0; category < MAX_DEVICE_CATEGORY; ++category) { + if (dc->categories & (1 << category)) { + error_printf(" \"%s\"", qdev_category_names[category]); + } + } + } + } if (dc->desc) { error_printf(", desc \"%s\"", dc->desc); } @@ -139,6 +172,23 @@ static const char *find_typename_by_alias(const char *alias) return NULL; } +static GSList *qdev_get_devices_by_category(unsigned long categories) +{ + 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 ((dc->categories & categories) == 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; @@ -148,7 +198,15 @@ 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; + PrintDevInfoData data = { &show_no_user, &category }; + + for (category = 0; category < MAX_DEVICE_CATEGORY; ++category) { + GSList *list = qdev_get_devices_by_category(1 << 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 RFC Made category a bitmap to support multifunction PCI devices include/hw/qdev-core.h | 15 ++++++++++++ qdev-monitor.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 75 insertions(+), 2 deletions(-)