[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] qemu-help: Sort devices by logical function
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] qemu-help: Sort devices by logical functionality |
Date: |
Sun, 28 Jul 2013 15:45:51 +0300 |
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 <address@hidden>
> ---
> 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
>