Hi:

I added some negative xml2argv tests as well as new xml2xml tests. In the process,
I also fixed a bug where I had not added appropriate code to generate the output
xml correctly.
The patch series is sent again as v2. Kindly, please provide inputs and review them.

[PATCH v2 1/4] pm/i386: add support for two options that controls
[PATCH v2 2/4] tests: add positive xml2argv tests to exercize pm acpi
[PATCH v2 3/4] tests: add negative xml2argv tests to exercize pm acpi
[PATCH v2 4/4] tests: add xml2xml tests to exercize pm acpi hotplug

On Thu, 19 Aug 2021, Ani Sinha wrote:

> Hi:
>
> I added some negative xml2argv tests as well as new xml2xml tests. In the process,
> I also fixed a bug where I had not added appropriate code to generate the output
> xml correctly.
> The patch series is sent again as v2. Kindly, please provide inputs and review them.
>
> [PATCH v2 1/4] pm/i386: add support for two options that controls
> [PATCH v2 2/4] tests: add positive xml2argv tests to exercize pm acpi
> [PATCH v2 3/4] tests: add negative xml2argv tests to exercize pm acpi
> [PATCH v2 4/4] tests: add xml2xml tests to exercize pm acpi hotplug

Ping ... please provide some review on this patchset.

Ping again …

On Wed, Sep 1, 2021 at 09:03 Ani Sinha <ani@anisinha.ca> wrote:

>
>
> On Thu, 19 Aug 2021, Ani Sinha wrote:
>
> > Hi:
> >
> > I added some negative xml2argv tests as well as new xml2xml tests. In
> the process,
> > I also fixed a bug where I had not added appropriate code to generate
> the output
> > xml correctly.
> > The patch series is sent again as v2. Kindly, please provide inputs and
> review them.
> >
> > [PATCH v2 1/4] pm/i386: add support for two options that controls
> > [PATCH v2 2/4] tests: add positive xml2argv tests to exercize pm acpi
> > [PATCH v2 3/4] tests: add negative xml2argv tests to exercize pm acpi
> > [PATCH v2 4/4] tests: add xml2xml tests to exercize pm acpi hotplug
>
> Ping ... please provide some review on this patchset.
>
>

On 8/19/21 6:50 AM, Ani Sinha wrote:
> Hi:
> 
> I added some negative xml2argv tests as well as new xml2xml tests. In the process,
> I also fixed a bug where I had not added appropriate code to generate the output
> xml correctly.
> The patch series is sent again as v2. Kindly, please provide inputs and review them.
> 
> [PATCH v2 1/4] pm/i386: add support for two options that controls
> [PATCH v2 2/4] tests: add positive xml2argv tests to exercize pm acpi
> [PATCH v2 3/4] tests: add negative xml2argv tests to exercize pm acpi
> [PATCH v2 4/4] tests: add xml2xml tests to exercize pm acpi hotplug


Hi,

Sorry for not sending email about this earlier. We went over a few 
things in an IRC chat a couple weeks ago. I'll reiterate the questions I 
asked there, and fill in more info on what we are looking for in patches.


* Usually when a new feature like this is supported, there will be 
several patches, divided something like this:


1) A patch that just adds the new QEMU capabilities flag(s) that will be 
used later to detect whether or not the selected QEMU binary supports 
the feature. (This might require updates to the sample QEMU capabilities 
test files, if the flag/option didn't happen to coincidentally already 
be in them. Instructions for regenerating the capabilities .replies 
files are in the comments of tests/qemucapabilitiestest.c.)


2) A patch adding the new XML to the RNG and to the XML parser in conf. 
This patch will also contain

  2b) an addition to XML documentation in docs/formatdomain.rst that 
names & describes the purpose of the new elements/attributes and 
provides at least one example of their use (the commit log message 
should include an example too, to make searching for the commit easier), and

  2c) parser/formatter tests added to qemuxml2xmltest.c, with the 
original XML in tests/qemuxml2argvdata, and output XML in qemuxml2xmlout 
(if the input and output files are identical, then the file in 
qemuxml2xmlout should be a symlink to the file in qemuxml2argvdata)


3) A patch that implements the backend of this new feature in the qemu 
driver by checking for its availability using the capabilities in patch 
1, and using the data in the domain object now being parsed by patch 2 
to add something to the qemu commandline. This patch should also include

   3b) all additional test cases for qemuxml2argvtest. By careful 
planning, these new test cases will use the same source XML that was 
added in (2c).

Note that *all* tests for new code are in the same patch as the new code 
itself. I like doing it this way because it ensures that the tests won't 
be forgotten/omitted on purpose when backporting to a different branch.

4) Oh, and just for abologna, a separate patch to add an entry to the 
NEWS.rst file for the next release :-)


0) (backing up a bit) In addition, the cover-letter (patch 0/n) should 
contain a *thorough* description of what each new XML element/attribute 
does, why it is desirable, and a link to the QEMU documentation (and/or 
patches as pushed into qemu) describing what they do in QEMU terms. I 
*think* that the 440fx-only option you added in these patches 
disables/enables hotplug of devices with a single systemwide flag 
(right?); I'm still uncertain what the other option does - apparently 
something about enabling the hotplug of a conventional PCI bridge? Or 
does it enable/disable hotplug of endpoint devices *into* conventional 
PCI bridges?).

This information could be included directly in the cover letter, or the 
cover letter can point to the commit log message for patch 2 or 3 which 
would then have all the information.


=========

About the "Why?" question above - many years ago someone decided that 
every feature added to QEMU *must* be supported directly in libvirt. 
This led to a large bloat of XML to support QEMU features that "seemed 
like a good idea at the time", but nobody ever uses (partly because it's 
unclear exactly how/when they should be used). In the more recent past, 
we've started asking "Is the maintenance burden of supporting this 
feature in libvirt really worthwhile? Is it usable? Who will actually 
use it, and for what?" (for a few years both QEMU and libvirt have been 
trying to get away from conventional PCI + 440fx, and concentrate our 
efforts on PCIe + Q35, so adding new functionality for conventional PCI 
+ 440fx feels kind of like adding a new option to the IDE disk 
controller :-)

I had questions on this topic for these new options, but realized that 
they all depended on my proper understanding of their function, and 
since I don't think I properly understand them yet, all my questions are 
potentially pointless, so I've removed them for now. Maybe it will all 
be clear once I've been properly informed.

====

In the meantime, although not officially "supported" (I believe its use 
will taint the libvirt config) it's possible to add any random option 
not supported by libvirt to the QEMU commandline with libvirt's 
<qemu:commandline> element, documented here:

 
http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html

If your reason for posting these patches was just so that you could try 
out these QEMU options, <qemu:commandline> is a much simpler/quicker way 
to do it than going through all the trouble of adding specific support 
to libvirt.

====

If you're beyond the experimenting stage with these new options, and 
really do have a real-world use case that requires specific support in 
libvirt, then I'd be happy to review (hopefully in a more timely 
fashion!) a resubmission of the patches organized as I laid out above, 
with the additional requested documentation added to 1) the cover 
letter, 2) the patch that adds the new XML to the parser/formatter, and 
3) formatdomain.rst.

On Thu, 9 Sep 2021, Laine Stump wrote:

> On 8/19/21 6:50 AM, Ani Sinha wrote:
> > Hi:
> >
> > I added some negative xml2argv tests as well as new xml2xml tests. In the
> > process,
> > I also fixed a bug where I had not added appropriate code to generate the
> > output
> > xml correctly.
> > The patch series is sent again as v2. Kindly, please provide inputs and
> > review them.
> >
> > [PATCH v2 1/4] pm/i386: add support for two options that controls
> > [PATCH v2 2/4] tests: add positive xml2argv tests to exercize pm acpi
> > [PATCH v2 3/4] tests: add negative xml2argv tests to exercize pm acpi
> > [PATCH v2 4/4] tests: add xml2xml tests to exercize pm acpi hotplug
>
>
> Hi,
>
> Sorry for not sending email about this earlier. We went over a few things in
> an IRC chat a couple weeks ago. I'll reiterate the questions I asked there,
> and fill in more info on what we are looking for in patches.

Thanks laine for such a detailed description of the preferred patch
organization. I have sent a v3 with the same subject line CC-ing you and
Julia. I have tried to add all the relevant details to the cover letter as
well as the commit messages. The patches has been organized as per the
suggestion in this email. I have tried to use my best judgement in terms
of description/documentation etc and look forward to your further
comments. I have also tried to answer the question as to why I think the
support is needed in libvirt. In particular, the libvirt passthrough to
qemu option has already been tried and tested to work.

Please let me know if you have further questions or doubts.

Ani


>
>
> * Usually when a new feature like this is supported, there will be several
> patches, divided something like this:
>
>
> 1) A patch that just adds the new QEMU capabilities flag(s) that will be used
> later to detect whether or not the selected QEMU binary supports the feature.
> (This might require updates to the sample QEMU capabilities test files, if the
> flag/option didn't happen to coincidentally already be in them. Instructions
> for regenerating the capabilities .replies files are in the comments of
> tests/qemucapabilitiestest.c.)
>
>
> 2) A patch adding the new XML to the RNG and to the XML parser in conf. This
> patch will also contain
>
>  2b) an addition to XML documentation in docs/formatdomain.rst that names &
> describes the purpose of the new elements/attributes and provides at least one
> example of their use (the commit log message should include an example too, to
> make searching for the commit easier), and
>
>  2c) parser/formatter tests added to qemuxml2xmltest.c, with the original XML
> in tests/qemuxml2argvdata, and output XML in qemuxml2xmlout (if the input and
> output files are identical, then the file in qemuxml2xmlout should be a
> symlink to the file in qemuxml2argvdata)
>
>
> 3) A patch that implements the backend of this new feature in the qemu driver
> by checking for its availability using the capabilities in patch 1, and using
> the data in the domain object now being parsed by patch 2 to add something to
> the qemu commandline. This patch should also include
>
>   3b) all additional test cases for qemuxml2argvtest. By careful planning,
> these new test cases will use the same source XML that was added in (2c).
>
> Note that *all* tests for new code are in the same patch as the new code
> itself. I like doing it this way because it ensures that the tests won't be
> forgotten/omitted on purpose when backporting to a different branch.
>
> 4) Oh, and just for abologna, a separate patch to add an entry to the NEWS.rst
> file for the next release :-)
>
>
> 0) (backing up a bit) In addition, the cover-letter (patch 0/n) should contain
> a *thorough* description of what each new XML element/attribute does, why it
> is desirable, and a link to the QEMU documentation (and/or patches as pushed
> into qemu) describing what they do in QEMU terms. I *think* that the
> 440fx-only option you added in these patches disables/enables hotplug of
> devices with a single systemwide flag (right?); I'm still uncertain what the
> other option does - apparently something about enabling the hotplug of a
> conventional PCI bridge? Or does it enable/disable hotplug of endpoint devices
> *into* conventional PCI bridges?).
>
> This information could be included directly in the cover letter, or the cover
> letter can point to the commit log message for patch 2 or 3 which would then
> have all the information.
>
>
> =========
>
> About the "Why?" question above - many years ago someone decided that every
> feature added to QEMU *must* be supported directly in libvirt. This led to a
> large bloat of XML to support QEMU features that "seemed like a good idea at
> the time", but nobody ever uses (partly because it's unclear exactly how/when
> they should be used). In the more recent past, we've started asking "Is the
> maintenance burden of supporting this feature in libvirt really worthwhile? Is
> it usable? Who will actually use it, and for what?" (for a few years both QEMU
> and libvirt have been trying to get away from conventional PCI + 440fx, and
> concentrate our efforts on PCIe + Q35, so adding new functionality for
> conventional PCI + 440fx feels kind of like adding a new option to the IDE
> disk controller :-)
>
> I had questions on this topic for these new options, but realized that they
> all depended on my proper understanding of their function, and since I don't
> think I properly understand them yet, all my questions are potentially
> pointless, so I've removed them for now. Maybe it will all be clear once I've
> been properly informed.
>
> ====
>
> In the meantime, although not officially "supported" (I believe its use will
> taint the libvirt config) it's possible to add any random option not supported
> by libvirt to the QEMU commandline with libvirt's <qemu:commandline> element,
> documented here:
>
>
> http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html
>
> If your reason for posting these patches was just so that you could try out
> these QEMU options, <qemu:commandline> is a much simpler/quicker way to do it
> than going through all the trouble of adding specific support to libvirt.
>
> ====
>
> If you're beyond the experimenting stage with these new options, and really do
> have a real-world use case that requires specific support in libvirt, then I'd
> be happy to review (hopefully in a more timely fashion!) a resubmission of the
> patches organized as I laid out above, with the additional requested
> documentation added to 1) the cover letter, 2) the patch that adds the new XML
> to the parser/formatter, and 3) formatdomain.rst.
>
>