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

[Michael S. Tsirkin]

On Wed, Aug 21, 2013 at 01:42:58PM +0300, Marcel Apfelbaum wrote:
> On Wed, 2013-08-21 at 11:23 +0200, Markus Armbruster wrote:
> > Marcel Apfelbaum <address@hidden> writes:
> > 
> > > On Tue, 2013-08-13 at 11:57 +0200, Markus Armbruster wrote:
> > >> This isn't patch review, just a couple of observations and questions.
> > >> 
> > >> Current use of categories, please correct misunderstandings:
> > >> 
> > >> * A device can have multiple categories.  Most (all?) devices currently
> > >>   have exactly one.
> > > All device have only one category for now.
> > > This is a preparation for multifunction devices.
> > >
> > >> 
> > >> * -device help shows categories, like this:
> > >> 
> > >>       name "NAME", bus "BUS", categories "CAT1" "CAT2"...
> > >> 
> > >> * -device help is sorted by category
> > >> 
> > >> * -device help shows the device once per category.  If the device has no
> > >>   categories, it's not shown at all.
> > >> 
> > >> Should we require devices to have at least one category?
> > > The whole idea of the patch was to help user navigating the command line 
> > > help.
> > > A device category will give a user at least a hint. 
> > 
> > Understand.
> > 
> > Devices without category are omitted from help.  That's not good.
> > Should we require devices to have at least one category?  Or should we
> > change help to show devices without a category?
> I prefer to require each device to have a category.
> The interesting part is how to enforce it.

What's hard? Can't we assert in init?

> > 
> > >> Eric, does libvirt still parse -device help?  If yes, can it cope with
> > >> the addition of "categories ..."?
> > > Also the "old" parsing mechanism would still work, it splits the raw
> > > by "," and looks for the key like "name".
> > >  
> > >> 
> > >> A possibly better way to group help by category: instead of adding
> > >> categories to each line, add category headlines, like this:
> > >> 
> > >>     Controller/Bridge/Hub devices:
> > >>     name "NAME", bus "BUS"...
> > >>     ...
> > >>     USB devices:
> > >>     name "NAME", bus "BUS"...
> > >>     ...
> > >>     Storage devices:
> > >>     ...
> > >> 
> > >> This way, showing devices with multiple categories once per category
> > >> actually makes sense.
> > > You are right. This is a very good "next step".
> > 
> > I'd love to see a patch from you :)
> On my to-do list ...
> 
> > 
> > >> DEVICE_CATEGORY_STORAGE comprises both storage controller devices
> > >> (providing storage buses such as IDE, SCSI) and storage devices
> > >> (plugging into such buses).  Some of our devices (*-fdc, virtio-blk)
> > >> integrate both in one device model[*].
> > > Yes, it does comprises both. It still helps the user that can now
> > > grep by this storage category and select from it rather than
> > > going on all the help.
> > >
> > >> 
> > >> DEVICE_CATEGORY_USB comprises *only* host controller devices (providing
> > >> USB bus(es)), *not* USB devices (plugging into USB bus).  These are
> > >> categorized by function instead:
> > > The "USB" is used here as a code-name rather than the BUS.
> > > It was never my intention to clone the bus type. It is already
> > > part of the description.
> > >
> > >> 
> > >> * DEVICE_CATEGORY_BRIDGE: usb-host, usb-hub
> > >> 
> > >> * DEVICE_CATEGORY_STORAGE: usb-bot, usb-uas, usb-storage
> > >> 
> > >> * DEVICE_CATEGORY_NETWORK: usb-bt-dongle, usb-net
> > >> 
> > >> * DEVICE_CATEGORY_INPUT: usb-kbd, usb-ccid, usb-wacom-tablet,
> > >>   usb-braille, usb-mouse, usb-serial
> > >> 
> > >> * DEVICE_CATEGORY_SOUND: usb-audio
> > >> 
> > >> * DEVICE_CATEGORY_MISC: usb-tablet, usb-redir
> > >> 
> > >> Should they additionally be DEVICE_CATEGORY_USB?
> > > As mentioned earlier, better if not (in my opinion.)
> > >
> > >> 
> > >> Why do we have DEVICE_CATEGORY_USB, but no categories for other buses,
> > >> like PCI or ISA?  Devices providing such buses are
> > >> DEVICE_CATEGORY_BRIDGE.  Why is USB different?
> > > Again, we already have the bus information, I was looking for
> > > functional info. "USB" was not used here as a BUS, but like a
> > > standalone "function".
> > 
> > Let me rephrase.  Why do we have a category for devices bridging to a
> > USB bus (USB host controllers), but don't have categories for devices
> > bridging to other buses?
> > 
> > Perhaps a possible answer is "because we have so many USB host
> > controllers, but usually only few to no user-selectable options for the
> > other buses".  Just thinking aloud; I'm not sure it's true.
> It is true, it was the exact purpose for it.
> 
> > 
> > >> Why is usb-host DEVICE_CATEGORY_BRIDGE?
> > > The category is named "Controller/Bridge/Hub" at command line
> > > I didn't want the name to be too long in the code.
> > 
> > I can't see how USB host device fits "Controller/Bridge/Hub"...
> I am open to suggestions. 

It might be a good idea to distinguish between
a host controller and a bridge.

> > 
> > The PCI device passhtrough devices kvm-pci-assign and vfio-pci are both
> > DEVICE_CATEGORY_MISC.
> Any problem with this? I think the Misc Category is appropriate for them.  
> 
> > 
> > >> Why is usb-tablet DEVICE_CATEGORY_MISC, but usb-wacom-tablet
> > >> DEVICE_CATEGORY_INPUT?
> > > This is a bug. Thanks for catching it!
> > 
> > You're welcome :)
> > 
> > Will you send a patch?
> Sure. Soon enough :)
> > 
> > >> DEVICE_CATEGORY_INPUT is weird.  Some devices in that category are truly
> > >> about input (usb-mouse, usb-kbd), others are at least as often used for
> > >> output (serial devices, PIOs)...
> > > It makes sense to rename it to "Input/Output".
> > 
> > Looks like the category comprises what we call character devices.
> > Perhaps not the friendliest term for casual users, but we already use it
> > in our documentation.
> And here is my problem. I (maybe) can infer from "char device" that
> it refers to input/output devices, but to expose it to user
> it is not user friendly/helpful in any way. The code constant may be
> DEVICE_CATEGORY_CHARACTER but we need a more meaningful name for the user.

At least serial devices should have their own category,
there are enough of these.

> > 
> > >> The difference between DEVICE_CATEGORY_INPUT and DEVICE_CATEGORY_MISC
> > >> seems unclear (see usb-tablet vs. usb-wacom-tablet above).
> > > Putting the bug aside, MISC is the category for devices that does
> > > not match a specific category.
> > >
> > >
> > > Thanks for the review Markus!
> > > The bottom line is that I wanted to help users in their adventure to form
> > > the command line by creating "Categories" that would split the 145 help 
> > > rows
> > > in batches of ~20. In this way the user can first select the desired 
> > > category and then choose the device.
> > 
> > Improvement, very much appreciated.
> Thanks
> 
> > 
> > >> [*] I hate that.
>