Re: [PATCH v2 0/4] Add support for two i386 pm options which control acpi hotplug

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.