9 a.m.
15/08/13 12:58, Daniel P. Berrange wrote:
On Thu, Aug 15, 2013 at 12:55:26PM +0200, anonym wrote:
> 13/08/13 16:15, Daniel P. Berrange wrote:
>> On Tue, Aug 13, 2013 at 01:52:33PM +0200, Fred A. Kemp wrote:
>>> From: "Fred A. Kemp" <anonym(a)riseup.net>
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index b811e1d..03fb798 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -8044,10 +8044,10 @@ qemuBuildCommandLine(virConnectPtr conn,
>>> bool withDeviceArg = false;
>>> bool deviceFlagMasked = false;
>>>
>>> - /* Unless we have -device, then USB disks need special
>>> - handling */
>>> + /* Unless we have `-device usb-storage`, then USB disks
>>> + need special handling */
>>> if ((disk->bus == VIR_DOMAIN_DISK_BUS_USB) &&
>>> - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
>>> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
>>> if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
>>> virCommandAddArg(cmd, "-usbdevice");
>>> virCommandAddArgFormat(cmd, "disk:%s",
disk->src);
>>
>> Hmm, I'm not sure this logic change is right.
>>
>> Previously if you have -device support, but 'usb-storage' was not
>> available, libvirt would try to start the guest with -device & then
>> QEMU would report an error.
>>
>> With this change, if you have -device support, but 'usb-storage' was
>> not available, then libvirt is going to fallback to using the legacy
>> '-usbdevice' support. This is not good.
>
> I fail to see why this is not good. I thought '-usbdevice' was the
> correct way do handle USB disks if 'usb-storage' is missing. Whether we
> have '-device' or seems beside the point; if we have 'usb-storage',
then
> it's implied we have '-device', and if we don't, '-device' is
worthless
> and '-usbdevice' is the way to go. Letting QEMU fail and die when we
> have '-device' but not 'usb-storage' seems worse than just falling
back
> to '-usbdevice'.
>
> What am I missing?
If -device exists, we must *never* use the -usbdevice syntax. This is
a legacy syntax that is only there for compatibility with apps that
predate the introduction of -device syntax.
Without knowing the exact development history of qemu, I assumed it
could be the case the we have '-device' but 'usb-storage' hadn't been
implemented yet (so '-usbdevice' was still the way to go).
If the 'usb-storage' device does not exist, then
'-usbdevice disk' is
not going to work either since it uses the same code internally.
Note that the QEMU_CAPS_DEVICE_USB_STORAGE capability I add only checks
if '-device usb-storage' is supported (by parsing 'qemu -device ?' like
other, similar caps) and has nothing to do with '-usbdevice'.
> Adding an explicit check for 'usb-storage' is a fine
goal, but we
> should be doing that check in the branch of this if() that handles
> '-device usb-storage', so we don't exercise the -usbdevice branch
So, what if I drop the above chunk, and do the following instead:
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8046,18 +8046,26 @@ qemuBuildCommandLine(virConnectPtr conn,
/* Unless we have -device, then USB disks need special
handling */
- if ((disk->bus == VIR_DOMAIN_DISK_BUS_USB) &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
- if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
- virCommandAddArg(cmd, "-usbdevice");
- virCommandAddArgFormat(cmd, "disk:%s", disk->src);
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
+ if (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_DEVICE_USB_STORAGE)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("This QEMU doesn't support
'-device "
+ "usb-storage'"));
+ goto error;
+ }
} else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unsupported usb disk type for
'%s'"),
- disk->src);
- goto error;
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+ virCommandAddArg(cmd, "-usbdevice");
+ virCommandAddArgFormat(cmd, "disk:%s", disk->src);
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unsupported usb disk type for
'%s'"),
+ disk->src);
+ goto error;
+ }
+ continue;
}
- continue;
}
switch (disk->device) {
Alternatively, I could do the following instead, which is more concise,
but doesn't happen in the same if() and thus spread the capability
checking over a larger code area:
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4311,13 +4311,6 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
goto error;
break;
case VIR_DOMAIN_DISK_BUS_USB:
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("This QEMU doesn't support '-device "
+ "usb-storage'"));
+ goto error;
+
+ }
virBufferAddLit(&opt, "usb-storage");
if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps)
< 0)
Once I have an opinion (or further explanation why I'm still confused
:)) I'll re-submit a new patchset with the preferred fix, rebased on the
then-current master.
Cheers!