Re: [libvirt] [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices
10:40 a.m.
Kevin Wolf <kwolf(a)redhat.com> writes:
Am 02.05.2013 um 15:41 hat Anthony Liguori geschrieben:
> Kevin Wolf <kwolf(a)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
7:31 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices
On Thu, May 02, 2013 at 10:40:06AM -0500, Anthony Liguori wrote:
Kevin Wolf <kwolf(a)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
--
|: 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 :|
8:52 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices
"Daniel P. Berrange" <berrange(a)redhat.com> writes:
On Thu, May 02, 2013 at 10:40:06AM -0500, Anthony Liguori wrote:
> Kevin Wolf <kwolf(a)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 :|
8:57 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices
On Fri, May 03, 2013 at 08:52:59AM -0500, Anthony Liguori wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> writes:
> On Thu, May 02, 2013 at 10:40:06AM -0500, Anthony Liguori wrote:
>> Kevin Wolf <kwolf(a)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
--
|: 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 :|
4222
days inactive
4223
days old
3 comments
2 participants
participants (2)
-
Anthony Liguori -
Daniel P. Berrange