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

[Kevin Wolf]

Am 02.05.2013 um 15:41 hat Anthony Liguori geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > Am 26.04.2013 um 21:43 hat Anthony Liguori geschrieben:
> >> +static void gd_block_device_menu_update(BlockDeviceMenu *bdm, BlockInfo 
> >> *info)
> >> +{
> >> +    bool value;
> >> +    const char *label = _("<No media>");
> >> +
> >> +    value = info->has_inserted && !info->locked;
> >
> > Shouldn't the actual value of info->inserted play a role as well?
> 
> inserted contains information about the inserted disk but doesn't
> contain a boolean to indicate that the device is inserted.
> 
> My understanding is that the existance of the inserted structure is what
> indicates that the device is inserted.

Sorry, my bad, I should have looked up the schema. I was indeed assuming
that it's a boolean.

> >> +static void gd_enum_disk(const char *path, const char *proptype, void 
> >> *opaque)
> >> +{
> >> +    GtkDisplayState *s = opaque;
> >> +    Object *obj;
> >> +    char *block_id;
> >> +
> >> +    obj = object_resolve_path(path, NULL);
> >> +    g_assert(obj != NULL);
> >> +
> >> +    block_id = object_property_get_str(obj, proptype, NULL);
> >> +    if (strcmp(block_id, "") != 0) {
> >> +        BlockDeviceMenu *bdm;
> >> +        DiskType disk_type;
> >> +        char *type;
> >> +        char *desc = NULL;
> >> +
> >> +        type = object_property_get_str(obj, "type", NULL);
> >> +
> >> +        if (strcmp(type, "ide-cd") == 0 || strcmp(type, "ide-hd") == 0) {
> >> +            desc = object_property_get_str(obj, "drive-id", NULL);
> >> +        } else {
> >> +            desc = g_strdup(type);
> >> +        }
> >
> > 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.

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".

I also wonder how other buses fit in your menu structure, like a SCSI
CD-ROM, or even PCI hotplug. Your proposal isn't quite consistent in
itself because it treats USB devices different from IDE or floppy
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.)

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"?

Anyway, none of this is actually an argument against having a property
for the human-readable name in the device, it's mostly about what should
be in there.

> > And then, of course, the question is still what a good human-readable
> > string is. A serial number generated by qemu, as we now get by default
> > for the CD-ROM, probably isn't. Something like "ATAPI CD-ROM at secondary
> > master" would probably be more helpful in this case.
> 
> I was going for how the device would be described in the guest such that
> if a user is looking at Device Manager in Windows or /dev/disk/by-id/ in
> Linux that there would be a clear association.

I see. We should probably display this somewhere, but it might not
always be the right name to interact with the user.

> >> +
> >> +        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.

Kevin