[Qemu-devel] [RFC PATCH 0/2] qemu-help: improve -device command line help

[Qemu-devel] [RFC PATCH 0/2] qemu-help: improve -device command line help

[Marcel Apfelbaum]

Running qemu with "-device ?" option returns ~145 lines.
It is hard to manage understanding the output.

Theses patches aim to partially solve the problem by dividing the devices
into logical categories like "Network/Display/..." and sorting them by it.

Marcel Apfelbaum (2):
  qemu-help: Sort devices by logical functionality
  devices: Associate devices to their logical category

 hw/audio/ac97.c         |  1 +
 hw/display/cirrus_vga.c |  1 +
 hw/net/eepro100.c       |  1 +
 hw/scsi/scsi-disk.c     |  4 ++++
 hw/usb/dev-hid.c        |  1 +
 include/hw/qdev-core.h  |  7 +++++++
 qdev-monitor.c          | 23 ++++++++++++++++++++++-
 7 files changed, 37 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 0/2] qemu-help: improve -device command line help

[Marcel Apfelbaum]

Running qemu with "-device ?" option returns ~145 lines.
It is hard to manage understanding the output.

Theses patches aim to partially solve the problem by dividing the devices
into logical categories like "Network/Display/..." and sorting them by it.

Marcel Apfelbaum (2):
  qemu-help: Sort devices by logical functionality
  devices: Associate devices to their logical category

 hw/audio/ac97.c         |  1 +
 hw/display/cirrus_vga.c |  1 +
 hw/net/eepro100.c       |  1 +
 hw/scsi/scsi-disk.c     |  4 ++++
 hw/usb/dev-hid.c        |  1 +
 include/hw/qdev-core.h  |  7 +++++++
 qdev-monitor.c          | 23 ++++++++++++++++++++++-
 7 files changed, 37 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 1/2] qemu-help: Sort devices by logical functionality

[Marcel Apfelbaum]

Categorize devices that appear as output to "-device ?" command
by logical functionality. Sort the devices by logical categories
before showing them to user.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 include/hw/qdev-core.h |  7 +++++++
 qdev-monitor.c         | 23 ++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 7fbffcb..4f7a9b8 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -17,6 +17,12 @@ 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 DEVICE_CATEGORY_STORAGE "storage"
+#define DEVICE_CATEGORY_NETWORK "network"
+#define DEVICE_CATEGORY_INPUT "input"
+#define DEVICE_CATEGORY_DISPLAY "display"
+#define DEVICE_CATEGORY_SOUND "sound"
+
 typedef int (*qdev_initfn)(DeviceState *dev);
 typedef int (*qdev_event)(DeviceState *dev);
 typedef void (*qdev_resetfn)(DeviceState *dev);
@@ -81,6 +87,7 @@ typedef struct DeviceClass {
     /*< public >*/
 
     const char *fw_name;
+    const char *category;
     const char *desc;
     Property *props;
     int no_user;
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e54dbc2..1446b6e 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -93,6 +93,9 @@ 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->category) {
+        error_printf(", category \"%s\"", dc->category);
+    }
     if (dc->desc) {
         error_printf(", desc \"%s\"", dc->desc);
     }
@@ -139,16 +142,34 @@ static const char *find_typename_by_alias(const char *alias)
     return NULL;
 }
 
+static gint qdev_device_compare(gconstpointer item1, gconstpointer item2)
+{
+    DeviceClass *dc1, *dc2;
+
+    dc1 = (DeviceClass *)object_class_dynamic_cast((ObjectClass *)item1,
+                                                   TYPE_DEVICE);
+    dc2 = (DeviceClass *)object_class_dynamic_cast((ObjectClass *)item2,
+                                                   TYPE_DEVICE);
+
+    return g_strcmp0(dc1->category, dc2->category);
+}
+
 int qdev_device_help(QemuOpts *opts)
 {
     const char *driver;
     Property *prop;
     ObjectClass *klass;
+    GSList *list;
 
     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);
+
+        list = object_class_get_list(TYPE_DEVICE, false);
+        list = g_slist_sort(list, qdev_device_compare);
+        g_slist_foreach(list, (GFunc)qdev_print_devinfo, &show_no_user);
+        g_slist_free(list);
+
         return 1;
     }
 
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 2/2] devices: Associate devices to their logical category

[Marcel Apfelbaum]

The category will be used to sort the devices displayed in
the command line help.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
Note that these are not all the needed changes, the only purpose of
this patch is to be a proof of concept.

 hw/audio/ac97.c         | 1 +
 hw/display/cirrus_vga.c | 1 +
 hw/net/eepro100.c       | 1 +
 hw/scsi/scsi-disk.c     | 4 ++++
 hw/usb/dev-hid.c        | 1 +
 5 files changed, 8 insertions(+)

diff --git a/hw/audio/ac97.c b/hw/audio/ac97.c
index 365b2f1..cbedf11 100644
--- a/hw/audio/ac97.c
+++ b/hw/audio/ac97.c
@@ -1420,6 +1420,7 @@ static void ac97_class_init (ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_INTEL_82801AA_5;
     k->revision = 0x01;
     k->class_id = PCI_CLASS_MULTIMEDIA_AUDIO;
+    dc->category = DEVICE_CATEGORY_SOUND;
     dc->desc = "Intel 82801AA AC97 Audio";
     dc->vmsd = &vmstate_ac97;
     dc->props = ac97_properties;
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index a440575..0f4be70 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -3002,6 +3002,7 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
     k->vendor_id = PCI_VENDOR_ID_CIRRUS;
     k->device_id = CIRRUS_ID_CLGD5446;
     k->class_id = PCI_CLASS_DISPLAY_VGA;
+    dc->category = DEVICE_CATEGORY_DISPLAY;
     dc->desc = "Cirrus CLGD 54xx VGA";
     dc->vmsd = &vmstate_pci_cirrus_vga;
     dc->props = pci_vga_cirrus_properties;
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index e0befb2..944a7ac 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -2084,6 +2084,7 @@ static void eepro100_class_init(ObjectClass *klass, void *data)
     info = eepro100_get_class_by_name(object_class_get_name(klass));
 
     dc->props = e100_properties;
+    dc->category = DEVICE_CATEGORY_NETWORK;
     dc->desc = info->desc;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->class_id = PCI_CLASS_NETWORK_ETHERNET;
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 74e6a14..3b2cd99 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2428,6 +2428,7 @@ static void scsi_hd_class_initfn(ObjectClass *klass, void *data)
     sc->alloc_req    = scsi_new_request;
     sc->unit_attention_reported = scsi_disk_unit_attention_reported;
     dc->fw_name = "disk";
+    dc->category = DEVICE_CATEGORY_STORAGE;
     dc->desc = "virtual SCSI disk";
     dc->reset = scsi_disk_reset;
     dc->props = scsi_hd_properties;
@@ -2457,6 +2458,7 @@ static void scsi_cd_class_initfn(ObjectClass *klass, void *data)
     sc->alloc_req    = scsi_new_request;
     sc->unit_attention_reported = scsi_disk_unit_attention_reported;
     dc->fw_name = "disk";
+    dc->category = DEVICE_CATEGORY_STORAGE;
     dc->desc = "virtual SCSI CD-ROM";
     dc->reset = scsi_disk_reset;
     dc->props = scsi_cd_properties;
@@ -2486,6 +2488,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data)
     sc->destroy      = scsi_destroy;
     sc->alloc_req    = scsi_block_new_request;
     dc->fw_name = "disk";
+    dc->category = DEVICE_CATEGORY_STORAGE;
     dc->desc = "SCSI block device passthrough";
     dc->reset = scsi_disk_reset;
     dc->props = scsi_block_properties;
@@ -2520,6 +2523,7 @@ static void scsi_disk_class_initfn(ObjectClass *klass, void *data)
     sc->alloc_req    = scsi_new_request;
     sc->unit_attention_reported = scsi_disk_unit_attention_reported;
     dc->fw_name = "disk";
+    dc->category = DEVICE_CATEGORY_STORAGE;
     dc->desc = "virtual SCSI disk or CD-ROM (legacy)";
     dc->reset = scsi_disk_reset;
     dc->props = scsi_disk_properties;
diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index 31f3cde..9d006ba 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -677,6 +677,7 @@ static void usb_mouse_class_initfn(ObjectClass *klass, void *data)
     uc->product_desc   = "QEMU USB Mouse";
     uc->usb_desc       = &desc_mouse;
     dc->vmsd = &vmstate_usb_ptr;
+    dc->category = DEVICE_CATEGORY_INPUT;
 }
 
 static const TypeInfo usb_mouse_info = {
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 0/2] qemu-help: improve -device command line help

[Paolo Bonzini]

Il 18/07/2013 10:27, Marcel Apfelbaum ha scritto:
> Running qemu with "-device ?" option returns ~145 lines.
> It is hard to manage understanding the output.
> 
> Theses patches aim to partially solve the problem by dividing the devices
> into logical categories like "Network/Display/..." and sorting them by it.

Nice! :)  An unexpected benefit of moving all the hw/ files to separate
per-type directory.

I checked libvirt and this should not break it.

Paolo

> Marcel Apfelbaum (2):
>   qemu-help: Sort devices by logical functionality
>   devices: Associate devices to their logical category
> 
>  hw/audio/ac97.c         |  1 +
>  hw/display/cirrus_vga.c |  1 +
>  hw/net/eepro100.c       |  1 +
>  hw/scsi/scsi-disk.c     |  4 ++++
>  hw/usb/dev-hid.c        |  1 +
>  include/hw/qdev-core.h  |  7 +++++++
>  qdev-monitor.c          | 23 ++++++++++++++++++++++-
>  7 files changed, 37 insertions(+), 1 deletion(-)
> 

Re: [Qemu-devel] [RFC PATCH 0/2] qemu-help: improve -device command line help

[Marcel Apfelbaum]

On Thu, 2013-07-18 at 15:56 +0200, Paolo Bonzini wrote:
> Il 18/07/2013 10:27, Marcel Apfelbaum ha scritto:
> > Running qemu with "-device ?" option returns ~145 lines.
> > It is hard to manage understanding the output.
> > 
> > Theses patches aim to partially solve the problem by dividing the devices
> > into logical categories like "Network/Display/..." and sorting them by it.
> 
> Nice! :)  An unexpected benefit of moving all the hw/ files to separate
> per-type directory.
Thanks Paolo,
> 
> I checked libvirt and this should not break it.
Me too :)
> 
> Paolo
> 
> > Marcel Apfelbaum (2):
> >   qemu-help: Sort devices by logical functionality
> >   devices: Associate devices to their logical category
> > 
> >  hw/audio/ac97.c         |  1 +
> >  hw/display/cirrus_vga.c |  1 +
> >  hw/net/eepro100.c       |  1 +
> >  hw/scsi/scsi-disk.c     |  4 ++++
> >  hw/usb/dev-hid.c        |  1 +
> >  include/hw/qdev-core.h  |  7 +++++++
> >  qdev-monitor.c          | 23 ++++++++++++++++++++++-
> >  7 files changed, 37 insertions(+), 1 deletion(-)
> > 
> 
> 

Re: [Qemu-devel] [RFC PATCH 0/2] qemu-help: improve -device command line help

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

On 07/18/2013 07:56 AM, Paolo Bonzini wrote:
> Il 18/07/2013 10:27, Marcel Apfelbaum ha scritto:
>> Running qemu with "-device ?" option returns ~145 lines.
>> It is hard to manage understanding the output.
>>
>> Theses patches aim to partially solve the problem by dividing the devices
>> into logical categories like "Network/Display/..." and sorting them by it.
> 
> Nice! :)  An unexpected benefit of moving all the hw/ files to separate
> per-type directory.
> 
> I checked libvirt and this should not break it.

Agreed - libvirt uses QMP queries so as to be immune to changes in
'-device help' output, since qemu 1.3 or so.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [RFC PATCH 0/2] qemu-help: improve -device command line help

[Andreas Färber]

Hi,

Am 18.07.2013 10:27, schrieb Marcel Apfelbaum:
> Running qemu with "-device ?" option returns ~145 lines.
> It is hard to manage understanding the output.
> 
> Theses patches aim to partially solve the problem by dividing the devices
> into logical categories like "Network/Display/..." and sorting them by it.
> 
> Marcel Apfelbaum (2):
>   qemu-help: Sort devices by logical functionality
>   devices: Associate devices to their logical category

Sorry to jump on this discussion. I think the idea to structure devices
better does make sense. However I feel that the implementation is not ideal:

For one, this adds a new field and uses it only for command line output,
while not benefiting libvirt or other QMP users.

For another, this adds a third tree overlay over devices:
We have the QOM inheritance tree with classifications such as PCIDevice,
ISADevice, VirtioDevice, USBDevice, etc.
We have Paolo's hw/ directory restructuring, which uses scsi, intc,
timer, etc. sub-directories.
The proposed category system is different from the sub-directories
(e.g., scsi/ vs STORAGE) and is completely manual, i.e. double work.

So I wonder if it would be possible to define the category per
Makefile.objs (whether in hw/Makefile.objs or in hw/*/Makefile.objs).

Or whether we can just reuse the type hierarchy and sort per base class
for now - the result would be different from the proposed categories but
hopefully reproducible elsewhere (assuming the base type is exposed in
QMP). Anthony had in the past suggested to change our inheritance tree
so that PCIDevice becomes an interface and the base class would be
specific to the chipset, i.e. might end up similar to the proposed
categories. (Getting rid of the parent field assumptions is a large
prerequisite for changing inheritance.)

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC PATCH 0/2] qemu-help: improve -device command line help

[Michael S. Tsirkin]

On Sun, Jul 21, 2013 at 02:09:20PM +0200, Andreas Färber wrote:
> Hi,
> 
> Am 18.07.2013 10:27, schrieb Marcel Apfelbaum:
> > Running qemu with "-device ?" option returns ~145 lines.
> > It is hard to manage understanding the output.
> > 
> > Theses patches aim to partially solve the problem by dividing the devices
> > into logical categories like "Network/Display/..." and sorting them by it.
> > 
> > Marcel Apfelbaum (2):
> >   qemu-help: Sort devices by logical functionality
> >   devices: Associate devices to their logical category
> 
> Sorry to jump on this discussion. I think the idea to structure devices
> better does make sense. However I feel that the implementation is not ideal:
> 
> For one, this adds a new field and uses it only for command line output,
> while not benefiting libvirt or other QMP users.

This is not a problem with implementation - that can be added on top.
I think HMP is a good start, that's where we have a real
pain point now, with basically no documentation, and it's
a less risky change with no compatibility commitments.

> For another, this adds a third tree overlay over devices:

Exactly. And that's by design since this information
is currently missing.

> We have the QOM inheritance tree with classifications such as PCIDevice,
> ISADevice, VirtioDevice, USBDevice, etc.
>
> We have Paolo's hw/ directory restructuring, which uses scsi, intc,
> timer, etc. sub-directories.
>
> The proposed category system is different from the sub-directories
> (e.g., scsi/ vs STORAGE) and is completely manual, i.e. double work.
>
> So I wonder if it would be possible to define the category per
> Makefile.objs (whether in hw/Makefile.objs or in hw/*/Makefile.objs).

This idea can't support multi-category devices.
To me, duplication seems minimal.

On the other hand plain C is much more readable than makefile based tricks.

> Or whether we can just reuse the type hierarchy and sort per base class
> for now - the result would be different from the proposed categories but
> hopefully reproducible elsewhere (assuming the base type is exposed in
> QMP).

What you suggest is mostly already available in the form of
"bus" field in -device help output.

But this misses the whole point which is to help users find
devices implementing a specific functionality.

> Anthony had in the past suggested to change our inheritance tree
> so that PCIDevice becomes an interface and the base class would be
> specific to the chipset, i.e. might end up similar to the proposed
> categories. (Getting rid of the parent field assumptions is a large
> prerequisite for changing inheritance.)
> 
> Regards,
> Andreas

As long as there are no plans to support multiple inheritance in QOM,
this does not seem feasible.

If category information does become available in QOM, it will be easy
to rip out an extra field, so I don't think we need to keep out
help text in the current messed-up state indefinitely until it's
available.

> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC PATCH 0/2] qemu-help: improve -device command line help

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC PATCH 0/2] qemu-help: improve -device command line help

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC PATCH 0/2] qemu-help: improve -device command line help

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

On 07/29/2013 02:24 PM, Anthony Liguori wrote:
> Applied.  Thanks.

Script botch-up?  This series has a title of RFC (which means it should
not be applied as-is), and there have been later revisions that dropped
the RFC in response to comments; did the correct version get applied?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]