Re: [libvirt] [RFC PATCH 3/6] qemu: Use helper functions for handling cmd line capabilities
> @@ -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