[libvirt] [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Anthony Liguori]

Kevin Wolf <kwolf at redhat.com> writes:

> Am 02.05.2013 um 15:41 hat Anthony Liguori geschrieben:
>> Kevin Wolf <kwolf at redhat.com> writes:
>> 
>> >
>> > Ugh. Comparing the device name to an incomplete set of strings here and
>> > then figuring out for each what the specific way for this device is to
>> > create a nice string sounds like a bad idea.
>> >
>> > Why can't all devices just expose a property with a human-readable
>> > string? We'll need it for more than just the disk change menus.
>> 
>> I thought about this, there are a few concerns.  The first is that you
>> might lose consistency across devices.  The second is i18n.
>
> What's the kind of consistency that you lose? I guess I could see the
> point (even if not agree) if it was about creating the string here vs.
> in each device, as the centralised strings would be more likely to use
> the same pattern, but you already have this part in the IDE device.

Note that these menu items are not descriptions of the device's class
but of the device.  They should be unique.

So for instance, for virtio-blk-pci, we would want something like:

"VirtIO Block Device  [00:03.0]"

If no serial property is defined on it,  If there is a serial property,
then the serial property should be shown vs. the PCI address.

We probably want to show the PCI address consistently for any PCI based
block device.  This is what I mean by consistency.  It's very hard to
enforce outside of a central location.

> The i18n point I don't buy. Avoiding i18n by choosing cryptic device
> names that can't be translated isn't a good strategy. But if you do have
> translations, it doesn't matter whether you have them in the GUI or in
> the device itself, except that the latter could be used outside the
> GTK frontend, too.
>
>> I would like to show USB device separately from IDE devices (even if
>> it's a USB CDROM).  I want the menu to look something like this:
>> 
>> QEMU DVD-ROM QM003 ->
>> Floppy Disk        ->
>> ---------------------
>> USB Devices        ->
>>                       USB Tablet                       ->
>>                       -----------------------------------
>>                       Description of USB Host Device 1 ->
>>                       Description of USB Host Device 2 ->
>>                       Description of USB Host Device 3 ->
>> 
>> Such that you can also do USB host device pass through via the menus.
>> 
>> From an i18n point of view, I would expect 'Floppy Disk' to be
>> translated.  I wouldn't expect 'QEMU DVD-ROM QM003' to be translated
>> though since this is how the device is described within the guest.
>
> Note that there can be two floppy drives. Currently both will show up as
> "isa-fdc".

No, it shows the BlockDriverState name which will always be unique.

> I also wonder how other buses fit in your menu structure, like a SCSI
> CD-ROM,

SCSI CD-ROM would show above the separator.

Note that this is only showing removable devices.  Hotplug isn't
considered here.  I was thinking in the long run we could have another
menu item under USB Devices called "Add/Remove Hardware..." that would
pop up a dialog to allow for hot plug/unplug.

> or even PCI hotplug. Your proposal isn't quite consistent in
> itself because it treats USB devices different from IDE or floppy
> devices.

That's a feature as these are the most common devices.

> (That said, I agree that CD and floppy should be accessible
> from the top level. But maybe usb-storage should be as well. It's not
> quite clear to me how things would work out best.)

I agree re: removable usb-storage.  The goal of the menu is to give
prominence to what the devices are that you would interact with the most.

>
> Another inconsistency is that you want to have "USB Tablet" there,
> because USB has a product description as well. Should this be "QEMU USB
> Tablet"?

Ack.  My intention was for that to be the product description FWIW.

>> >> +
>> >> +        if (strcmp(type, "ide-cd") == 0) {
>> >> +            disk_type = DT_CDROM;
>> >> +        } else if (strcmp(type, "isa-fdc") == 0) {
>> >> +            disk_type = DT_FLOPPY;
>> >> +        } else {
>> >> +            disk_type = DT_NORMAL;
>> >> +        }
>> >
>> > Same thing here, comparing against strings is a hack. Devices should
>> > probably have a property that says what kind of device they are.
>> 
>> Ack, this is nasty.  I would like to eliminate this.  There is a type
>> field in BlockInfo but:
>> 
>> # @type: This field is returned only for compatibility reasons, it should
>> #        not be used (always returns 'unknown')
>> 
>> I vaguely remember this happening but I don't remember the specific
>> reason why.  I would definitely prefer that we filled out type
>> correctly.
>> 
>> I think Markus was involved in this.  Markus or Luiz, do you remember
>> the story here?
>
> The reason is that BlockInfo is about the backend and it simply doesn't
> know (ever since we introduced if=none, this was buggy, so we just
> abandoned it at some point). We would have to ask the device, not the
> block layer.

Yes, this makes sense.  We could introduce an interface that all disks
implemented that returned information about whether it was a CD-ROM,
Floppy, etc.

How does libvirt cope with this today?  I presume they do something
similar to what this patch is doing in terms of hard coding device
names.

Regards,

Anthony Liguori

>
> Kevin