Re: [libvirt] [RFC v3 0/4] update NVDIMM support

No need to CC developers for libvirt, we're all subscribed to the list anyways and generally are faithful in reading. Reviews are a different story. On 12/12/18 7:52 AM, Luyao Zhong wrote: <sigh> https://libvirt.org/hacking.html ... git send-email --cover-letter --no-chain-reply-to --annotate \ --confirm=always --to=libvir-list(a)redhat.com master You've missed the '--no-chain-reply-to'... In order to test how something would look on the list, alter the above "--to" to be yourself and see how things would look. Seems this more of a v3 and less of an RFC, true? Still based on what I know, there'll need to be a v4 anyway. You don't hand modify the .replies and .xml files - they are generated via tools, see the end of tests/qemucapabilitiestest.c for details. For the two caps you've added - one (*ALIGN) was introduced in qemu.git commit 9837684. It's already represented in the *2.12*.replies file, search on "memory-backend-file" and then the "align". The other (*PMEM) was introduced in qemu.git commit a4de8552. It's already represented in the *3.1*.replies file, using a similar search except for "pmem". As for the 3rd feature w/ nvdimm flags, well that's a little different than a memory-backend-file object attribute. The nvdimm is a -device and so you need to follow a different model - see other 'static struct virQEMUCapsStringFlags virQEMUCapsDeviceProps*'. You may need to go look at some previous commits to find examples that will generate the .replies/.xml changes to create a flag. Look for "-device xxx,yyy=zzz..." type changes Not entirely - mostly the current problem is how I believe you generated what you have. First, there's a slightly different naming scheme for the caps_latest files, but you can git mv what you have... $ git mv tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.x86_64-latest.args $ git mv tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.x86_64-latest.args $ git mv tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.x86_64-latest.args for what you have as of patch3 will use the new/right naming scheme. Additionally when using the CAPS_LATEST option that means you're going to get "all" the caps enabled. IOW: just copying some existing similar output that doesn't use all the caps or CAPS_LATEST and modifying it to suit the needs of your change probably won't work very well because the output will include *all* the caps changes not just "some". So, if I was starting from scratch and just adding a new .args file, then what I usually do after "building" the patch with the qemu_command and qemuxml2argvtest changes is: $ touch tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.x86_64-latest.arg Repeat the touch for as many new tests you add - if you don't then running the test will fail and tell you the output file doesn't exist. Then use: $ VIR_TEST_REGENERATE_OUTPUT=1 tests/qemuxml2argvtest to "create" the new output. Then look at the output to make sure it has what I want. So let's see how you probably did this... Looking at the differences between your new XML files in patch1, I assume that you just copied: tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml then renamed to suit each test while changing each appropriately, since: $ diff tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml 49d48 < <alignsize unit='KiB'>2048</alignsize> $ diff tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml 49d48 < <pmem/> $ diff tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml 53d52 < <unarmed/> $ and likewise for the .args files (NB: my command is after the git mv) $ diff tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.x86_64-latest.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.args 15c15 < share=no,size=536870912,align=2097152 \ --- $ diff tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.x86_64-latest.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.args 15c15 < share=no,size=536870912,pmem=on \ --- $ diff tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.x86_64-latest.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.args 16c16 < -device nvdimm,node=0,unarmed=on,memdev=memnvdimm0,id=nvdimm0,slot=0 \ --- So if I now run: $ VIR_TEST_REGENERATE_OUTPUT=1 tests/qemuxml2argvtest TEST: qemuxml2argvtest ........................................ 40 ........................................ 80 ........................................ 120 ........................................ 160 ........................................ 200 ........................................ 240 ........................................ 280 ........................................ 320 ........................................ 360 ........................................ 400 ........................................ 440 ........................................ 480 ........................................ 520 ........................................ 560 ........................................ 600 ........................................ 640 ........................................ 680 ........................................ 720 ........................................ 760 ....................!!!................. 800 ........................................ 840 ........................................ 880 ....... 887 FAIL You'll note the FAIL and the 3 ! (bangs). That indicates 3 tests that have changed which a git diff will show. If you search that output for a difference on what changed in each of your new commands, you'll note that specific output for the nvdimm didn't change only other things were adjusted (which are all normal for a full capability run). I'll provide some feedback in the other patches, but you still have a bit to go. John