[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/2] qemu: Enable QEMU_CAPS_PCI_ROMBAR unconditionally



On Fri, 2016-05-13 at 13:51 -0400, Cole Robinson wrote:
> > 
> > +/* Capabilities that we assume are always enabled
> > + * for QEMU >= 0.12.0 */
> > +static void
> > +virQEMUCapsInitHelpBasic(virQEMUCapsPtr qemuCaps)
> > +{
> > +
> > +    /* Although very new versions of qemu advertise the presence of
> > +     * the rombar option in the output of "qemu -device pci-assign,?",
> > +     * this advertisement was added to the code long after the option
> > +     * itself. According to qemu developers, though, rombar is
> > +     * available in all qemu binaries from release 0.12 onward.
> > +     * Setting the capability this way makes it available in more
> > +     * cases where it might be needed, and shouldn't cause any false
> > +     * positives (in the case that it did, qemu would produce an error
> > +     * log and refuse to start, so it would be immediately obvious).
> > +     */
> > +    virQEMUCapsSet(qemuCaps, QEMU_CAPS_PCI_ROMBAR);
> > +
> > +    virQEMUCapsSet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO);
> > +    virQEMUCapsSet(qemuCaps, QEMU_CAPS_CPU_HOST);
> > +}
> 
> As Pavel said, the approach we've taken for other flags is to rename them to
> X_$NAME, and remove all code usages. Moving these flags to their own function
> confuses that approach, so I suggest either give the X_$NAME treatment to each
> of those flags, or just using this series to disable PCI_ROMBAR and leave the
> other flags for a later series.

I hadn't noticed that pattern, and it makes much more sense
than what I'm doing here - setting a capability unconditionally
and never checking for its presence seemed kinda off, glad you
guys caught it and suggested a better solution before I had a
chance to push :)

-- 
Andrea Bolognani
Software Engineer - Virtualization Team


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]