* [Qemu-devel] [PATCH 1/2] Mostly revert "qemu-help: Sort devices by logical functionality"
2013-10-10 13:00 [Qemu-devel] [PATCH 0/2] Improve -device command line help some more armbru
@ 2013-10-10 13:00 ` armbru
2013-10-10 14:56 ` Marcel Apfelbaum
2013-10-10 13:00 ` [Qemu-devel] [PATCH 2/2] qdev-monitor: Group "device_add help" and "info qdm" by category armbru
2013-10-10 14:56 ` [Qemu-devel] [PATCH 0/2] Improve -device command line help some more Marcel Apfelbaum
2 siblings, 1 reply; 6+ messages in thread
From: armbru @ 2013-10-10 13:00 UTC (permalink / raw)
To: qemu-devel; +Cc: anthony, marcel.a
From: Markus Armbruster <armbru@redhat.com>
This reverts most of commit 3d1237fb2ab4edb926c717767bb5e31d6053a7c5.
The commit claims to sort the output of "-device help" "by
functionality rather than alphabetical". Issues:
* The output was unsorted before, not alphabetically sorted.
Misleading, but harmless enough.
* The commit doesn't just sort the output of "-device help" as it
claims, it adds categories to each line of "-device help", and it
prints devices once per category. In particular, devices without a
category aren't shown anymore. Maybe such devices should not exist,
but they do. Regression.
* Categories are also added to the output of "info qdm". Silent
change, not nice. Output remains unsorted, unlike "-device help".
I'm going to reimplement the feature we actually want, without the
warts. Reverting the flawed commit first should make it easier to
review. However, I can't revert it completely, since DeviceClass
member categories has been put to use. So leave that part in.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/hw/qdev-core.h | 16 ----------------
qdev-monitor.c | 48 +++++++++---------------------------------------
2 files changed, 9 insertions(+), 55 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index a62f231..e191ca0 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -30,22 +30,6 @@ typedef enum DeviceCategory {
DEVICE_CATEGORY_MAX
} DeviceCategory;
-static inline const char *qdev_category_get_name(DeviceCategory category)
-{
- static const char *category_names[DEVICE_CATEGORY_MAX] = {
- [DEVICE_CATEGORY_BRIDGE] = "Controller/Bridge/Hub",
- [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);
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 410cdcb..e5adf6c 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -75,27 +75,24 @@ static bool qdev_class_has_alias(DeviceClass *dc)
return (qdev_class_get_alias(dc) != NULL);
}
-static void qdev_print_class_devinfo(DeviceClass *dc)
+static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
{
- DeviceCategory category;
+ DeviceClass *dc;
+ bool *show_no_user = opaque;
+
+ dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
- if (!dc) {
+ if (!dc || (show_no_user && !*show_no_user && dc->no_user)) {
return;
}
- error_printf("name \"%s\"", object_class_get_name(OBJECT_CLASS(dc)));
+ error_printf("name \"%s\"", object_class_get_name(klass));
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);
}
@@ -105,15 +102,6 @@ static void qdev_print_class_devinfo(DeviceClass *dc)
error_printf("\n");
}
-static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
-{
- DeviceClass *dc;
-
- dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
-
- qdev_print_class_devinfo(dc);
-}
-
static int set_property(const char *name, const char *value, void *opaque)
{
DeviceState *dev = opaque;
@@ -151,21 +139,6 @@ 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 (!dc->no_user && test_bit(category, dc->categories)) {
- qdev_print_class_devinfo(dc);
- }
- }
- g_slist_free(list);
-}
-
int qdev_device_help(QemuOpts *opts)
{
const char *driver;
@@ -174,11 +147,8 @@ int qdev_device_help(QemuOpts *opts)
driver = qemu_opt_get(opts, "driver");
if (driver && is_help_option(driver)) {
- DeviceCategory category;
- for (category = 0; category < DEVICE_CATEGORY_MAX; ++category) {
- qdev_print_category_devices(category);
- }
-
+ bool show_no_user = false;
+ object_class_foreach(qdev_print_devinfo, TYPE_DEVICE, false, &show_no_user);
return 1;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] Mostly revert "qemu-help: Sort devices by logical functionality"
2013-10-10 13:00 ` [Qemu-devel] [PATCH 1/2] Mostly revert "qemu-help: Sort devices by logical functionality" armbru
@ 2013-10-10 14:56 ` Marcel Apfelbaum
2013-10-11 6:41 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Marcel Apfelbaum @ 2013-10-10 14:56 UTC (permalink / raw)
To: armbru; +Cc: qemu-devel, anthony
On Thu, 2013-10-10 at 15:00 +0200, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
>
> This reverts most of commit 3d1237fb2ab4edb926c717767bb5e31d6053a7c5.
>
> The commit claims to sort the output of "-device help" "by
> functionality rather than alphabetical". Issues:
>
> * The output was unsorted before, not alphabetically sorted.
> Misleading, but harmless enough.
I meant that will be sorted, but the sorting will be by functionality
rather than alphabetical. I can understand the confusion.
>
> * The commit doesn't just sort the output of "-device help" as it
> claims, it adds categories to each line of "-device help", and it
> prints devices once per category. In particular, devices without a
> category aren't shown anymore. Maybe such devices should not exist,
> but they do. Regression.
Yes - :(, it was a drawback that devices with no category are not
printed. On the other hand all devices must be attached to a category,
otherwise we will have again a lot of devices with no category.
I suppose your approach lets us at least with the possibility to see
these devices giving us a chance to attach them to their category.
>
> * Categories are also added to the output of "info qdm". Silent
> change, not nice. Output remains unsorted, unlike "-device help".
I checked libvirt and the parsing of the output is not affected
by adding the category.
I still think that adding the category in each line may be
useful when using grep, but I suppose that we can grep by category
with -A x.
Thanks,
Marcel
> I'm going to reimplement the feature we actually want, without the
> warts. Reverting the flawed commit first should make it easier to
> review. However, I can't revert it completely, since DeviceClass
> member categories has been put to use. So leave that part in.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/hw/qdev-core.h | 16 ----------------
> qdev-monitor.c | 48 +++++++++---------------------------------------
> 2 files changed, 9 insertions(+), 55 deletions(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index a62f231..e191ca0 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -30,22 +30,6 @@ typedef enum DeviceCategory {
> DEVICE_CATEGORY_MAX
> } DeviceCategory;
>
> -static inline const char *qdev_category_get_name(DeviceCategory category)
> -{
> - static const char *category_names[DEVICE_CATEGORY_MAX] = {
> - [DEVICE_CATEGORY_BRIDGE] = "Controller/Bridge/Hub",
> - [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);
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 410cdcb..e5adf6c 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -75,27 +75,24 @@ static bool qdev_class_has_alias(DeviceClass *dc)
> return (qdev_class_get_alias(dc) != NULL);
> }
>
> -static void qdev_print_class_devinfo(DeviceClass *dc)
> +static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
> {
> - DeviceCategory category;
> + DeviceClass *dc;
> + bool *show_no_user = opaque;
> +
> + dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
>
> - if (!dc) {
> + if (!dc || (show_no_user && !*show_no_user && dc->no_user)) {
> return;
> }
>
> - error_printf("name \"%s\"", object_class_get_name(OBJECT_CLASS(dc)));
> + error_printf("name \"%s\"", object_class_get_name(klass));
> 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);
> }
> @@ -105,15 +102,6 @@ static void qdev_print_class_devinfo(DeviceClass *dc)
> error_printf("\n");
> }
>
> -static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
> -{
> - DeviceClass *dc;
> -
> - dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
> -
> - qdev_print_class_devinfo(dc);
> -}
> -
> static int set_property(const char *name, const char *value, void *opaque)
> {
> DeviceState *dev = opaque;
> @@ -151,21 +139,6 @@ 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 (!dc->no_user && test_bit(category, dc->categories)) {
> - qdev_print_class_devinfo(dc);
> - }
> - }
> - g_slist_free(list);
> -}
> -
> int qdev_device_help(QemuOpts *opts)
> {
> const char *driver;
> @@ -174,11 +147,8 @@ int qdev_device_help(QemuOpts *opts)
>
> driver = qemu_opt_get(opts, "driver");
> if (driver && is_help_option(driver)) {
> - DeviceCategory category;
> - for (category = 0; category < DEVICE_CATEGORY_MAX; ++category) {
> - qdev_print_category_devices(category);
> - }
> -
> + bool show_no_user = false;
> + object_class_foreach(qdev_print_devinfo, TYPE_DEVICE, false, &show_no_user);
> return 1;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] Mostly revert "qemu-help: Sort devices by logical functionality"
2013-10-10 14:56 ` Marcel Apfelbaum
@ 2013-10-11 6:41 ` Markus Armbruster
0 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2013-10-11 6:41 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: qemu-devel, anthony
Marcel Apfelbaum <marcel.a@redhat.com> writes:
> On Thu, 2013-10-10 at 15:00 +0200, armbru@redhat.com wrote:
>> From: Markus Armbruster <armbru@redhat.com>
>>
>> This reverts most of commit 3d1237fb2ab4edb926c717767bb5e31d6053a7c5.
>>
>> The commit claims to sort the output of "-device help" "by
>> functionality rather than alphabetical". Issues:
>>
>> * The output was unsorted before, not alphabetically sorted.
>> Misleading, but harmless enough.
> I meant that will be sorted, but the sorting will be by functionality
> rather than alphabetical. I can understand the confusion.
Happens :)
>> * The commit doesn't just sort the output of "-device help" as it
>> claims, it adds categories to each line of "-device help", and it
>> prints devices once per category. In particular, devices without a
>> category aren't shown anymore. Maybe such devices should not exist,
>> but they do. Regression.
> Yes - :(, it was a drawback that devices with no category are not
> printed. On the other hand all devices must be attached to a category,
> otherwise we will have again a lot of devices with no category.
s/otherweise will have again/but unfortunately we still have/
> I suppose your approach lets us at least with the possibility to see
> these devices giving us a chance to attach them to their category.
Yes.
If you want to enforce "no uncategorized devices", you need to catch
them reliably at compile time or "make check" time. Once that's done,
we can simplify my qdev_print_class_devinfos() not to look for such
devices. Patches welcome :)
>> * Categories are also added to the output of "info qdm". Silent
>> change, not nice. Output remains unsorted, unlike "-device help".
>
> I checked libvirt and the parsing of the output is not affected
> by adding the category.
I didn't claim actual harm, just pointed out that you should've
mentioned it in the commit message, and that the reasons for improving
"device_add help" equally apply to "info qdm".
> I still think that adding the category in each line may be
> useful when using grep, but I suppose that we can grep by category
> with -A x.
Yes, repeating the categories on each line can be convenient when the
lines are fed to programs, but help output is primarily for humans.
Try 'sed -n "/^$cat devices:/,/^\$/p"' instead of 'grep $cat'.
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] qdev-monitor: Group "device_add help" and "info qdm" by category
2013-10-10 13:00 [Qemu-devel] [PATCH 0/2] Improve -device command line help some more armbru
2013-10-10 13:00 ` [Qemu-devel] [PATCH 1/2] Mostly revert "qemu-help: Sort devices by logical functionality" armbru
@ 2013-10-10 13:00 ` armbru
2013-10-10 14:56 ` [Qemu-devel] [PATCH 0/2] Improve -device command line help some more Marcel Apfelbaum
2 siblings, 0 replies; 6+ messages in thread
From: armbru @ 2013-10-10 13:00 UTC (permalink / raw)
To: qemu-devel; +Cc: anthony, marcel.a
From: Markus Armbruster <armbru@redhat.com>
Output is a long, unsorted list. Not very helpful. Print one list
per device category instead, with a header line identifying the
category, plus a list of uncategorized devices. Print each list in
case-insenitive alphabetical order.
Devices with multiple categories are listed multiple times.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qdev-monitor.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 53 insertions(+), 14 deletions(-)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e5adf6c..a02c925 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -75,18 +75,9 @@ 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_devinfo(DeviceClass *dc)
{
- 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;
- }
-
- 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);
}
@@ -102,6 +93,55 @@ static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
error_printf("\n");
}
+static gint devinfo_cmp(gconstpointer a, gconstpointer b)
+{
+ return strcasecmp(object_class_get_name((ObjectClass *)a),
+ object_class_get_name((ObjectClass *)b));
+}
+
+static void qdev_print_devinfos(bool show_no_user)
+{
+ static const char *cat_name[DEVICE_CATEGORY_MAX + 1] = {
+ [DEVICE_CATEGORY_BRIDGE] = "Controller/Bridge/Hub",
+ [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",
+ [DEVICE_CATEGORY_MAX] = "Uncategorized",
+ };
+ GSList *list, *elt;
+ int i;
+ bool cat_printed;
+
+ list = g_slist_sort(object_class_get_list(TYPE_DEVICE, false),
+ devinfo_cmp);
+
+ for (i = 0; i <= DEVICE_CATEGORY_MAX; i++) {
+ cat_printed = false;
+ for (elt = list; elt; elt = elt->next) {
+ DeviceClass *dc = OBJECT_CLASS_CHECK(DeviceClass, elt->data,
+ TYPE_DEVICE);
+ if ((i < DEVICE_CATEGORY_MAX
+ ? !test_bit(i, dc->categories)
+ : !bitmap_empty(dc->categories, DEVICE_CATEGORY_MAX))
+ || (!show_no_user && dc->no_user)) {
+ continue;
+ }
+ if (!cat_printed) {
+ error_printf("%s%s devices:\n", i ? "\n" : "",
+ cat_name[i]);
+ cat_printed = true;
+ }
+ qdev_print_devinfo(dc);
+ }
+ }
+
+ g_slist_free(list);
+}
+
static int set_property(const char *name, const char *value, void *opaque)
{
DeviceState *dev = opaque;
@@ -147,8 +187,7 @@ 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);
+ qdev_print_devinfos(false);
return 1;
}
@@ -587,7 +626,7 @@ void do_info_qtree(Monitor *mon, const QDict *qdict)
void do_info_qdm(Monitor *mon, const QDict *qdict)
{
- object_class_foreach(qdev_print_devinfo, TYPE_DEVICE, false, NULL);
+ qdev_print_devinfos(true);
}
int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 0/2] Improve -device command line help some more
2013-10-10 13:00 [Qemu-devel] [PATCH 0/2] Improve -device command line help some more armbru
2013-10-10 13:00 ` [Qemu-devel] [PATCH 1/2] Mostly revert "qemu-help: Sort devices by logical functionality" armbru
2013-10-10 13:00 ` [Qemu-devel] [PATCH 2/2] qdev-monitor: Group "device_add help" and "info qdm" by category armbru
@ 2013-10-10 14:56 ` Marcel Apfelbaum
2 siblings, 0 replies; 6+ messages in thread
From: Marcel Apfelbaum @ 2013-10-10 14:56 UTC (permalink / raw)
To: armbru; +Cc: qemu-devel, anthony
On Thu, 2013-10-10 at 15:00 +0200, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
>
> Marcel's recent improvements (commit dbd94f8..125ee0e) go in the right
> direction, but there are issues (see PATCH 1/2), and I find the
> resulting help output still hard to read.
>
> This series redoes the help printing part of Marcel's series. Result
> looks like this (moxie picked as example for brevity):
>
> $ qemu-system-moxie -device help
> Controller/Bridge/Hub devices:
> name "usb-host", bus usb-bus
> name "usb-hub", bus usb-bus
>
> Storage devices:
> name "scsi-block", bus SCSI, desc "SCSI block device passthrough"
> name "scsi-cd", bus SCSI, desc "virtual SCSI CD-ROM"
> name "scsi-disk", bus SCSI, desc "virtual SCSI disk or CD-ROM (legacy)"
> name "scsi-generic", bus SCSI, desc "pass through generic scsi device (/dev/sg*)"
> name "scsi-hd", bus SCSI, desc "virtual SCSI disk"
>
> Input devices:
> name "isa-serial", bus ISA
> name "usb-kbd", bus usb-bus
> name "usb-mouse", bus usb-bus
> name "usb-tablet", bus usb-bus
>
> Misc devices:
> name "smbus-eeprom", bus i2c-bus
> name "usb-redir", bus usb-bus
>
> Additionally, "info qdm" is again just like "device_add help" with
> no-user devices included.
I think that adding the category in each line may be
useful when using grep, but I suppose that we can grep by category
with -A x.
Thanks for continuing to improve the help output.
Marcel,
Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
>
> Markus Armbruster (2):
> Mostly revert "qemu-help: Sort devices by logical functionality"
> qdev-monitor: Group "device_add help" and "info qdm" by category
>
> include/hw/qdev-core.h | 16 ----------
> qdev-monitor.c | 85 ++++++++++++++++++++++++++++----------------------
> 2 files changed, 47 insertions(+), 54 deletions(-)
>
^ permalink raw reply [flat|nested] 6+ messages in thread