@@ -3181,12 +3181,18 @@ qemuBuildCommandLine(virConnectPtr conn, devices. Fortunately, those don't need static PCI addresses, so we don't really care that we can't use -device */ - if ((qemuCmdFlags & QEMU_CAPS_DEVICE) && - (disk->bus != VIR_DOMAIN_DISK_BUS_XEN)) - withDeviceArg = 1; - if (!(optstr = qemuBuildDriveStr(disk, bootable, - (withDeviceArg ? qemuCmdFlags : - (qemuCmdFlags & ~QEMU_CAPS_DEVICE))))) + if (qemuCapsGet(qemuCmdFlags, QEMU_CAPS_DEVICE)) { + if (disk->bus != VIR_DOMAIN_DISK_BUS_XEN) { + withDeviceArg = 1; + } else { + qemuCapsClear(&qemuCmdFlags, QEMU_CAPS_DEVICE); + deviceFlagMasked = true; + } + } + optstr = qemuBuildDriveStr(disk, bootable, qemuCmdFlags); + if (deviceFlagMasked) + qemuCapsSet(&qemuCmdFlags, QEMU_CAPS_DEVICE); + if (!optstr)
Certainly a tougher prospect. On the surface, it looks correct; however, I'm worried that there might be some potential for a data race bug - that is, if we ever have any command that can build a command line string while holding a read-only connection, is it right to be temporarily modifying the domain def? (That is, it seems like domxml-to-native should work on a read-only connection without trying to modify the guest definition.)
Eh, the code is not changing domain def anywhere. The only thing which is changed is qemuCmdFlags, which is generated in every caller of qemuBuildCommandLine() so cloning the bitmap seemed like an unneeded overkill to me.
Would it be any easier to just change the signature of qemuBuildDriveStr to take an additional bool parameter on whether to ignore the QEMU_CAPS_DEVICE flag?
That's another possibility.
My concerns about qemuBuildDriveStr probably warrant a v2 patch (perhaps broken into two portions - separating a parameter addition or cloning from the mechanical rewrite into helper functions).
Do you still think it's needed? Jirka