| Message ID | 1367005387-330-4-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
On Thu, 02 May 2013 08:41:50 -0500 Anthony Liguori <aliguori@us.ibm.com> wrote: > >> + > >> + 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? IIRC, we had a type field which was a string and Markus eliminated it because it was unreliable. I was afraid that dropping fields from a QMP output would be incompatible, so Markus maintained the field but it's always set to 'unknown'. It seems totally fine to me to have a new field with the device type as an enum, but of course it has to be reliable. PS: For more information about the reliableness of this field please contact Markus :)
Am 02.05.2013 um 15:41 hat Anthony Liguori geschrieben: > Kevin Wolf <kwolf@redhat.com> 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
Kevin Wolf <kwolf@redhat.com> writes: > Am 02.05.2013 um 15:41 hat Anthony Liguori geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 26.04.2013 um 21:43 hat Anthony Liguori geschrieben: [...] >> >> + >> >> + 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. Correct. commit d8aeeb31d53a07a0cce36c7bcf53684953c2e445 Author: Markus Armbruster <armbru@redhat.com> Date: Mon May 16 15:04:55 2011 +0200 block QMP: Deprecate query-block's "type", drop info block's "type=" query-block's specification documents response member "type" with values "hd", "cdrom", "floppy", "unknown". Its value is unreliable: a block device used as floppy has type "floppy" if created with if=floppy, but type "hd" if created with if=none. That's because with if=none, the type is at best a declaration of intent: the drive can be connected to any guest device. Its type is really the guest device's business. Reporting it here is wrong. No known user of QMP uses "type". It's unlikely that any unknown users exist, because its value is useless unless you know how the block device was created. But then you also know the true value. Fixing the broken value risks breaking (hypothetical!) clients that somehow rely on the current behavior. Not fixing the value risks breaking (hypothetical!) clients that rely on the value to be accurate. Can't entirely avoid hypothetical lossage. Change the value to be always "unknown". This makes "info block" always report "type=unknown". Pointless. Change it to not report the type. Signed-off-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Kevin Wolf <kwolf@redhat.com> writes: > Am 02.05.2013 um 15:41 hat Anthony Liguori geschrieben: >> Kevin Wolf <kwolf@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
Kevin Wolf <kwolf@redhat.com> writes: > Am 02.05.2013 um 15:41 hat Anthony Liguori geschrieben: >> Kevin Wolf <kwolf@redhat.com> 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". This is a bug. My intention was to use block_id as the description, not type. Regards, Anthony Liguori
On Thu, May 02, 2013 at 10:40:06AM -0500, Anthony Liguori wrote: > Kevin Wolf <kwolf@redhat.com> writes: > >> >> + > >> >> + 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. Sorry, not really sure what your question is here - how does libvirt cope with what exactly ? Daniel
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Thu, May 02, 2013 at 10:40:06AM -0500, Anthony Liguori wrote: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> >> + >> >> >> + 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. > > Sorry, not really sure what your question is here - how does libvirt > cope with what exactly ? Given a device, how do you figure out if it's a cdrom/floppy/whatever without hard coding a mapping of class name -> device type. Pretty sure libvirt just has a class name mapping, right? Regards, Anthony Liguori > > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Fri, May 03, 2013 at 08:52:59AM -0500, Anthony Liguori wrote: > "Daniel P. Berrange" <berrange@redhat.com> writes: > > > On Thu, May 02, 2013 at 10:40:06AM -0500, Anthony Liguori wrote: > >> Kevin Wolf <kwolf@redhat.com> writes: > >> >> >> + > >> >> >> + 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. > > > > Sorry, not really sure what your question is here - how does libvirt > > cope with what exactly ? > > Given a device, how do you figure out if it's a cdrom/floppy/whatever > without hard coding a mapping of class name -> device type. > > Pretty sure libvirt just has a class name mapping, right? The only place where we'd ever need todo that is when we reverse engineer a libvirt XML config from a set of QEMU command line args. For that we just look at the if=XXX parameter currently. Our reverse engineering code is currently broken for if=none scenarios, due mostly to our laziness in writing code to parse the corresponding -device arg. Daniel
diff --git a/ui/gtk.c b/ui/gtk.c index e12f228..c420914 100644 --- a/ui/gtk.c +++ b/ui/gtk.c @@ -64,6 +64,7 @@ #include "x_keymap.h" #include "keymaps.h" #include "sysemu/char.h" +#include "qapi/qmp/qevents.h" //#define DEBUG_GTK @@ -139,6 +140,10 @@ typedef struct GtkDisplayState GtkWidget *grab_on_hover_item; GtkWidget *vga_item; + GtkWidget *devices_menu_item; + GtkWidget *devices_menu; + GHashTable *devices_map; + int nb_vcs; VirtualConsole vc[MAX_VCS]; @@ -165,6 +170,8 @@ typedef struct GtkDisplayState bool external_pause_update; bool modifier_pressed[ARRAY_SIZE(modifier_keycode)]; + + Notifier event_notifier; } GtkDisplayState; static GtkDisplayState *global_state; @@ -1382,6 +1389,296 @@ static GtkWidget *gd_create_menu_view(GtkDisplayState *s, GtkAccelGroup *accel_g return view_menu; } +typedef void (DeviceIterFunc)(const char *path, const char *property, void *opaque); + +static void device_foreach_proptype(const char *path, const char *proptype, + DeviceIterFunc *fn, void *opaque) +{ + ObjectPropertyInfoList *prop_list, *iter; + + prop_list = qmp_qom_list(path, NULL); + for (iter = prop_list; iter; iter = iter->next) { + ObjectPropertyInfo *prop = iter->value; + + if (strstart(prop->type, "child<", NULL)) { + char *subpath; + subpath = g_strdup_printf("%s/%s", path, prop->name); + device_foreach_proptype(subpath, proptype, fn, opaque); + g_free(subpath); + } else if (strcmp(prop->type, proptype) == 0) { + fn(path, prop->name, opaque); + } + } + qapi_free_ObjectPropertyInfoList(prop_list); +} + +typedef enum DiskType +{ + DT_NORMAL, + DT_CDROM, + DT_FLOPPY, +} DiskType; + +typedef struct BlockDeviceMenu +{ + GtkWidget *entry; + GtkWidget *submenu; + GtkWidget *eject; + GtkWidget *change; + + char *name; + char *path; + char *desc; + DiskType disk_type; +} BlockDeviceMenu; + +static void gd_block_device_menu_update(BlockDeviceMenu *bdm, BlockInfo *info) +{ + bool value; + const char *label = _("<No media>"); + + value = info->has_inserted && !info->locked; + 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_update_block_menus(GtkDisplayState *s) +{ + BlockInfoList *block_list, *iter; + + block_list = qmp_query_block(NULL); + for (iter = block_list; iter; iter = iter->next) { + BlockInfo *info = iter->value; + BlockDeviceMenu *bdm; + + if (!info->removable) { + continue; + } + + bdm = g_hash_table_lookup(s->devices_map, info->device); + if (!bdm) { + continue; + } + + gd_block_device_menu_update(bdm, info); + } + qapi_free_BlockInfoList(block_list); +} + +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); + } + + 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; + } + + 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); +} + +static void gd_error(GtkDisplayState *s, const char *fmt, ...) +{ + GtkWidget *dialog; + char *message; + va_list ap; + + va_start(ap, fmt); + message = g_strdup_vprintf(fmt, ap); + va_end(ap); + + dialog = gtk_message_dialog_new(GTK_WINDOW(s->window), GTK_DIALOG_DESTROY_WITH_PARENT, + GTK_MESSAGE_ERROR, GTK_BUTTONS_OK, + "%s", message); + + g_free(message); + + gtk_widget_show_all(dialog); + + g_signal_connect_swapped(dialog, "response", G_CALLBACK(gtk_widget_destroy), dialog); +} + +static void gd_menu_bdm_change_response(GtkDialog *dialog, gint response_id, + gpointer opaque) +{ + BlockDeviceMenu *bdm = opaque; + + if (response_id == GTK_RESPONSE_ACCEPT) { + char *filename; + Error *local_err = NULL; + + filename = gtk_file_chooser_get_filename(GTK_FILE_CHOOSER(dialog)); + + qmp_change(bdm->name, filename, false, NULL, &local_err); + if (local_err) { + gd_error(global_state, + _("Block device change failed: %s"), + error_get_pretty(local_err)); + error_free(local_err); + } + + g_free(filename); + } + + gtk_widget_destroy(GTK_WIDGET(dialog)); +} + +static void gd_menu_bdm_change(GtkMenuItem *item, void *opaque) +{ + GtkDisplayState *s = global_state; + BlockDeviceMenu *bdm = opaque; + GtkWidget *chooser; + + chooser = gtk_file_chooser_dialog_new(_("Choose Image File"), + GTK_WINDOW(s->window), + GTK_FILE_CHOOSER_ACTION_OPEN, + GTK_STOCK_CANCEL, GTK_RESPONSE_CANCEL, + GTK_STOCK_OPEN, GTK_RESPONSE_ACCEPT, + NULL); + + g_signal_connect(chooser, "response", + G_CALLBACK(gd_menu_bdm_change_response), bdm); + + gtk_widget_show_all(chooser); +} + +static void gd_menu_bdm_eject(GtkMenuItem *item, void *opaque) +{ + BlockDeviceMenu *bdm = opaque; + + qmp_eject(bdm->name, false, false, NULL); +} + +static void gd_event_notify(Notifier *notifier, void *data) +{ + QDict *event = qobject_to_qdict(data); + const char *event_str; + + event_str = qdict_get_str(event, "event"); + if (strcmp(event_str, "DEVICE_TRAY_MOVED") == 0) { + gd_update_block_menus(global_state); + } +} + +static GtkWidget *gd_create_menu_devices(GtkDisplayState *s, GtkAccelGroup *accel_group) +{ + GtkWidget *devices_menu; + BlockInfoList *block_list, *iter; + + s->event_notifier.notify = gd_event_notify; + qmp_add_event_notifier(&s->event_notifier); + s->devices_map = g_hash_table_new(g_str_hash, g_str_equal); + + /* We first search for devices that has a drive property. */ + device_foreach_proptype("/", "drive", gd_enum_disk, s); + + devices_menu = gtk_menu_new(); + gtk_menu_set_accel_group(GTK_MENU(devices_menu), accel_group); + + block_list = qmp_query_block(NULL); + for (iter = block_list; iter; iter = iter->next) { + BlockInfo *info = iter->value; + BlockDeviceMenu *bdm; + const char *stock_id = NULL; + + /* Only show removable devices */ + if (!info->removable) { + continue; + } + + /* Only show devices were we could identify the actual device */ + bdm = g_hash_table_lookup(s->devices_map, info->device); + if (!bdm) { + continue; + } + + bdm->submenu = gtk_menu_new(); + bdm->change = gtk_image_menu_item_new_from_stock(GTK_STOCK_OPEN, NULL); + gtk_menu_item_set_label(GTK_MENU_ITEM(bdm->change), _("<No media>")); + bdm->eject = gtk_image_menu_item_new_from_stock(GTK_STOCK_CLOSE, NULL); + gtk_menu_item_set_label(GTK_MENU_ITEM(bdm->eject), _("Eject")); + gtk_menu_shell_append(GTK_MENU_SHELL(bdm->submenu), bdm->change); + gtk_menu_shell_append(GTK_MENU_SHELL(bdm->submenu), bdm->eject); + + g_signal_connect(bdm->change, "activate", + G_CALLBACK(gd_menu_bdm_change), bdm); + g_signal_connect(bdm->eject, "activate", + G_CALLBACK(gd_menu_bdm_eject), bdm); + + switch (bdm->disk_type) { + case DT_NORMAL: + stock_id = GTK_STOCK_HARDDISK; + break; + case DT_CDROM: + stock_id = GTK_STOCK_CDROM; + break; + case DT_FLOPPY: + stock_id = GTK_STOCK_FLOPPY; + break; + } + + bdm->entry = gtk_image_menu_item_new_from_stock(stock_id, NULL); + gtk_menu_item_set_label(GTK_MENU_ITEM(bdm->entry), bdm->desc); + gtk_menu_item_set_submenu(GTK_MENU_ITEM(bdm->entry), bdm->submenu); + gtk_menu_shell_append(GTK_MENU_SHELL(devices_menu), bdm->entry); + + gd_block_device_menu_update(bdm, info); + } + + qapi_free_BlockInfoList(block_list); + + return devices_menu; +} + static void gd_create_menus(GtkDisplayState *s) { GtkAccelGroup *accel_group; @@ -1389,6 +1686,7 @@ static void gd_create_menus(GtkDisplayState *s) accel_group = gtk_accel_group_new(); s->machine_menu = gd_create_menu_machine(s, accel_group); s->view_menu = gd_create_menu_view(s, accel_group); + s->devices_menu = gd_create_menu_devices(s, accel_group); s->machine_menu_item = gtk_menu_item_new_with_mnemonic(_("_Machine")); gtk_menu_item_set_submenu(GTK_MENU_ITEM(s->machine_menu_item), @@ -1399,6 +1697,10 @@ static void gd_create_menus(GtkDisplayState *s) gtk_menu_item_set_submenu(GTK_MENU_ITEM(s->view_menu_item), s->view_menu); gtk_menu_shell_append(GTK_MENU_SHELL(s->menu_bar), s->view_menu_item); + s->devices_menu_item = gtk_menu_item_new_with_mnemonic(_("_Devices")); + gtk_menu_item_set_submenu(GTK_MENU_ITEM(s->devices_menu_item), s->devices_menu); + gtk_menu_shell_append(GTK_MENU_SHELL(s->menu_bar), s->devices_menu_item); + g_object_set_data(G_OBJECT(s->window), "accel_group", accel_group); gtk_window_add_accel_group(GTK_WINDOW(s->window), accel_group); s->accel_group = accel_group;
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> --- ui/gtk.c | 302 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 302 insertions(+)