Re: [libvirt] [PATCH] conf: Introduce align for hostmem-file

[removed developers from To since they read libvir-list anyway] On 05/29/2018 08:38 AM, Jie Wang wrote: Would perhaps be nice to have a few more details about what this is as part of the commit message... and the expected usage. I see that "align" was added in 2.12 by commit id '98376843', so you'll need to add a capability for the switch; otherwise, supplying align=XXX to a pre-2.12 emulator would probably fail, correct? See libvirt commit '72c1770aa0' done for "memory-backend-file.discard-data" for the methodology, but I expect the next round to contain: { "align", QEMU_CAPS_OBJECT_MEMORY_FILE_ALIGN }, in virQEMUCapsObjectPropsMemoryBackendFile along with changes to the various tests/qemucapabilitiesdata/caps_2.12*.xml files which you can generate via "VIR_TEST_REGENERATE_OUTPUT=1 tests/qemucapabilitiestest" once you have compiled the qemu_capabilities.{c,h} changes. Split the patches into domain_conf, formatdomain, xml2xml testing as one patch and qemu_command, qemuxml2argv tests in another patch. In between the two patches, sandwich in the qemu_capabilities changes. You'll also need a patch 4 to adjust docs/news.xml to briefly describe the enhancement. Before reposting, please be sure to pass the "make [-j N] check syntax-check" as check fails with: FAIL: virschematest FAIL: qemuxml2xmltest and syntax-check fails with (at least): tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml maint.mk: empty line(s) or no newline at EOF make: *** [maint.mk:930: sc_prohibit_empty_lines_at_EOF] Error 1 make: *** Waiting for unfinished jobs.... s/sieze/size/ [1] see notes regarding formatting... I see you copied the NVDIMM target size description, but I think it's better described using something like: "The size of the XXXX. The value by default is in kilobytes, but the unit attribute can be used to scale the value." Search around the formatdomain.html.in for other uses of size for examples. Beyond that, the description in/from the qemu commit regarding how this is used, might be good to at least paraphrase in the libvirt description. Strange to just have 1 element in a list - in that case, just indicate that for QEMU domains the alignment size must be... BTW: I would have expected in some qemu_domain post parse function to see a VIR_ROUND_UP for this or a comparison forcing failure because the value isn't aligned properly. Hint, qemuDomainGetMemorySizeAlignment [1] This should just follow the "pagesize" value. There's no need for: <align> <size unit='KiB'>2048</size> </align> it should be : <align unit='KiB'>2048</align> and it should be a child of <source>, just like <pagesize> Thus changing: <group> <element name="path"> <ref name="absFilePath"/> </element> </group> to be: <group> <interleave> <element name="path"> <ref name="absFilePath"/> </element> <optional> <element name="align"> <ref name="scaledInteger"/> </element> </optional> </interleave> </group> That way it's *very obvious* that this align is only for nvdimm This then moves into the source parsing. I prefer, def->align > 0 - the value isn't a bool or a pointer. and all of this moves into source def format Should virDomainMemoryDefCheckABIStability ensure that align sizes between @src and @dst match? Use cscope to search the sources for VIR_DOMAIN_MEMORY_MODEL_NVDIMM to make sure there's no other places that may need adjustment... I think you probably need a change to the virDomainMemoryDefValidate to check if align is supplied for memory types other than nvdimm and then fail, alt Since this alignment only for NVDIMM, it should be renamed to nvdimmAlign and put under nvdimmPath. If you use "U:align" you don't need the mem->align condition. [1] From above will change this... Use DO_TEST_CAPS_LATEST This isn't a link to the file - the contents of the file is a link... That's probably why schematest and xml2xmltest fail John No need to CC me on the next series, I follow the list. As time permits in my schedule, I do reviews unless someone else gets to them before me.