On Tue, May 12, 2020 at 10:26:46 -0400, Chris Jester-Young wrote: > On Mon, May 11, 2020 at 09:13:42AM +0200, Peter Krempa wrote: > > It would be great if you could describe that 'pvscsi' indeed is the > > vmware paravirtual scsi controller even in qemu, because I had to dig > > through the code to figure it out. > > Sure, just in the commit message? Or elsewhere too? Commit message is probably enough. > > Can the 'pvscsi' device be compiled out using any upstream way? If not > > we don't really need a capability for it since qemu-1.5.3 is the oldest > > supported qemu and all other versions support it as well. > > As Daniel P. Berrangé mentioned, this can be configured out via Kconfig. > Plus there are actually some files in tests/qemucapabilitiesdata that do > not advertise that device (e.g., *.s390x.replies). Yes, thus we need the capability. I'd slightly prefer if the capability is added in a separate commit but that's not strictly required. > > > The remainder of the patch looks okay to me, but it's missing a > > qemuxml2argvtest case for the new controller. It looks like the > > controller is using PCI addressing so the rest should be okay. > > Sure thing, I'll be happy to add a test. Do I just take one of the > existing tests and modify the XML and expected resultant command? Both are actually fine. If there is an appropriate file where the data would fit, just adding it to an existing test case is appropriate. Otherwise you can copy a minimal test case and add the controller. We prefer if new tests use DO_TEST_CAPS_LATEST, or the version-locked variants of the test macro so that we are testing a "real" situation. It also simplifies addition of the test. Note that you can use VIR_TEST_REGENERATE_OUTPUT=1 env variable when running the qemuxml2argvtest to force creation of the output file and then just verify that it's as expected. > > If you figure out that the capability is required, I'd prefer if the > > addition of the capability is in a separate patch. > > Can do. Given the (hopefully, given the patches from earlier today) > impending arrival of merge requests, and the fact that I already have > to redo the patch (since new capabilities have since been added), my > current plan is to just post the new commits in a merge request. > > (Don't worry, I won't actually file the MR until the CI system is in > place.) Well, CI is already there, but the main libvirt project still works on the mail based workflow
