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