Kevin Wolf <kw...@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 <aligu...@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
