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

[Anthony Liguori <aliguori@us.ibm.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 26.04.2013 um 21:43 hat Anthony Liguori geschrieben:
>> To generate this menu, we first walk the composition tree to
>> find any device with a 'drive' property.  We then record these
>> devices and the BlockDriverState that they are associated with.
>> 
>> Then we use query-block to get the BDS state for each of the
>> discovered devices.
>> 
>> This code doesn't handle hot-plug yet but it should deal nicely
>> with someone using the human monitor.
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> I haven't checked what causes it, but with this patch applied I get a
> screenful of GTK error messages when I exit qemu with Alt-F4.

I think I know what this is.  I assume we're getting an event after the
window is no longer realized.

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

>> +    gtk_widget_set_sensitive(bdm->eject, value);
>> +
>> +    value = !info->locked;
>> +    gtk_widget_set_sensitive(bdm->change, value);
>> +
>> +    if (info->has_inserted) {
>> +        label = info->inserted->file;
>> +        if (strlen(label) > 32) {
>> +            char *new_label;
>> +
>> +            new_label = strrchr(label, '/');
>> +            if (new_label) {
>> +                label = new_label + 1;
>> +            }
>> +        }
>> +    }
>> +
>> +    gtk_menu_item_set_label(GTK_MENU_ITEM(bdm->change), label);
>> +}
>
>> +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.

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.

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

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

Regards,

Anthony Liguori

>
>> +        bdm = g_malloc0(sizeof(*bdm));
>> +        bdm->name = g_strdup(block_id);
>> +        bdm->path = g_strdup(path);
>> +        bdm->desc = desc;
>> +        bdm->disk_type = disk_type;
>> +
>> +        g_free(type);
>> +
>> +        g_hash_table_insert(s->devices_map, bdm->name, bdm);
>> +    }
>> +    g_free(block_id);
>> +}
>
> Kevin