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

[Kevin Wolf <kwolf@redhat.com>]

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.

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

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

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.

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

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