On 02/09/2011 09:02 AM, Jiri Denemark wrote:
> ---
No comment in the commit message about the name of the new helper methods?
> src/qemu/qemu_capabilities.c | 152 ++++++++++++++----------
> src/qemu/qemu_capabilities.h | 9 ++
> src/qemu/qemu_command.c | 260 +++++++++++++++++++++--------------------
> src/qemu/qemu_driver.c | 24 ++--
> src/qemu/qemu_hotplug.c | 86 +++++++-------
> tests/qemuhelptest.c | 2 +-
> tests/qemuxml2argvtest.c | 4 +-
> 7 files changed, 288 insertions(+), 249 deletions(-)
>
> cmd = virCommandNewArgList(qemu, "-cpu", "?", NULL);
> - if (qemuCmdFlags & QEMU_CAPS_NODEFCONFIG)
> + if (qemuCapsGet(qemuCmdFlags, QEMU_CAPS_NODEFCONFIG))
Looks like a pretty trivial conversion. To prove that I really reviewed
this, let's see if I can find the few non-mechanical conversions :)
> /* Prefer -chardev spicevmc (detected earlier) over -device spicevmc */
> - if (!(*flags & QEMU_CAPS_CHARDEV_SPICEVMC) &&
> + if (!qemuCapsGet(*flags, QEMU_CAPS_CHARDEV_SPICEVMC) &&
This was probably one that needed attention; looks correct.
> +void
> +qemuCapsSet(unsigned long long *caps,
> + enum qemuCapsFlags flag)
> +{
> + *caps |= flag;
> +}
> +
> +
> +void
> +qemuCapsClear(unsigned long long *caps,
> + enum qemuCapsFlags flag)
> +{
> + *caps &= ~flag;
> +}
> +
> +
> +bool
> +qemuCapsGet(unsigned long long caps,
> + enum qemuCapsFlags flag)
> +{
> + return !!(caps & flag);
> +}
Nice and simple.
> - if ((qemuCmdFlags & QEMU_CAPS_KVM) &&
> - (def->virtType == VIR_DOMAIN_VIRT_QEMU) &&
> - (qemuCmdFlags & QEMU_CAPS_DRIVE_BOOT))
> - qemuCmdFlags -= QEMU_CAPS_DRIVE_BOOT;
> + if (qemuCapsGet(qemuCmdFlags, QEMU_CAPS_KVM) &&
> + (def->virtType == VIR_DOMAIN_VIRT_QEMU))
> + qemuCapsClear(&qemuCmdFlags, QEMU_CAPS_DRIVE_BOOT);
There's another non-mechanical change. The new logic is one line
shorter, and still correct.
> @@ -3141,12 +3140,13 @@ qemuBuildCommandLine(virConnectPtr conn,
> int bootable = 0;
> virDomainDiskDefPtr disk = def->disks[i];
> int withDeviceArg = 0;
> + bool deviceFlagMasked = false;
> int j;
>
> /* Unless we have -device, then USB disks need special
> handling */
> if ((disk->bus == VIR_DOMAIN_DISK_BUS_USB) &&
> - !(qemuCmdFlags & QEMU_CAPS_DEVICE)) {
> + !qemuCapsGet(qemuCmdFlags, QEMU_CAPS_DEVICE)) {
> if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> virCommandAddArg(cmd, "-usbdevice");
> virCommandAddArgFormat(cmd, "disk:%s", disk->src);
> @@ -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.)
Should we instead be cloning the bitset, and passing the clone to
qemuBuildDriveStr, rather than changing and restoring the original?
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?
> @@ -3444,8 +3450,8 @@ qemuBuildCommandLine(virConnectPtr conn,
> *
> * NB, no support for -netdev without use of -device
> */
> - if ((qemuCmdFlags & QEMU_CAPS_NETDEV) &&
> - (qemuCmdFlags & QEMU_CAPS_DEVICE)) {
> + if (qemuCapsGet(qemuCmdFlags, QEMU_CAPS_NETDEV) &&
> + qemuCapsGet(qemuCmdFlags, QEMU_CAPS_DEVICE)) {
I'm actually kind of surprised that we didn't have more cases of:
if (qemuCmdFlags & (QEMU_CAPS_NETDEV | QEMU_CAPS_DEVICE))
Thankfully, not having expressions like that pre-patch made it easier
for you to rewrite things during this patch.
Sadly, the compiler could probably optimize the prepatch expression into
that simpler code, while it cannot "see through" the function call for
the same optimization post-patch. On the other hand, it's not much of a
speed regression, and is essential for when we cross the 64-bit
boundary, so I don't mind the optimization loss.
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).
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list