[libvirt] [RFC PATCH 3/6] qemu: Use helper functions for handling cmd line capabilities

[Jiri Denemark]

> > @@ -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