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.
