On Tue, Apr 24, 2018 at 16:29:45 -0400, John Ferlan wrote: > > > On 04/24/2018 03:28 AM, Peter Krempa wrote: > > On Mon, Apr 23, 2018 at 20:00:04 -0400, John Ferlan wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1149445 > >> > >> If the domain requests usage of the genid functionality, > >> then add the QEMU '-device vmgenid' to the command line > >> providing either the supplied or generated GUID value. > >> > >> Add tests for both a generated and supplied GUID value. > >> > >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> > >> --- > >> src/qemu/qemu_command.c | 31 +++++++++++++++++++++++++++++++ > >> tests/qemuxml2argvdata/genid-auto.args | 24 ++++++++++++++++++++++++ > >> tests/qemuxml2argvdata/genid.args | 24 ++++++++++++++++++++++++ > >> tests/qemuxml2argvtest.c | 4 ++++ > >> 4 files changed, 83 insertions(+) > >> create mode 100644 tests/qemuxml2argvdata/genid-auto.args > >> create mode 100644 tests/qemuxml2argvdata/genid.args > >> > >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > >> index b666f3715f..1f5e79d86a 100644 > >> --- a/src/qemu/qemu_command.c > >> +++ b/src/qemu/qemu_command.c > >> @@ -5942,6 +5942,34 @@ qemuBuildSmbiosCommandLine(virCommandPtr cmd, > >> > >> > >> static int > >> +qemuBuildVMGenIDCommandLine(virCommandPtr cmd, > >> + const virDomainDef *def, > >> + virQEMUCapsPtr qemuCaps) > >> +{ > >> + virBuffer opts = VIR_BUFFER_INITIALIZER; > >> + char guid[VIR_UUID_STRING_BUFLEN]; > >> + > >> + if (!def->genidRequested) > >> + return 0; > >> + > >> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMGENID)) { > >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > >> + _("genid is not supported with this QEMU binary")); > >> + return -1; > > > > This is already checked in qemuProcessGenID. > > > > Oh right. > > >> + } > >> + > >> + virUUIDFormat(def->genid, guid); > >> + virBufferAsprintf(&opts, "vmgenid,guid=%s,id=vmgenid0", guid); > > > > [...] > > > >> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > >> index 74d930ebe2..0dd0850036 100644 > >> --- a/tests/qemuxml2argvtest.c > >> +++ b/tests/qemuxml2argvtest.c > >> @@ -805,6 +805,10 @@ mymain(void) > >> QEMU_CAPS_SECCOMP_BLACKLIST); > >> DO_TEST_PARSE_ERROR("minimal-no-memory", NONE); > >> DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP); > >> + > >> + DO_TEST("genid", QEMU_CAPS_DEVICE_VMGENID); > >> + DO_TEST("genid-auto", QEMU_CAPS_DEVICE_VMGENID); > > > > Please use DO_TEST_CAPS_LATEST/DO_TEST_CAPS_VER > > > > OK - That was added after I originally posted. Happy, happy, joy, joy, > genid is the guinea pig! Still, it's not clear what would be expected > since the comments indicate desired usage for positive test cases. > > Since pre 2.9 would be a failure scenario, the only difference is works > or doesn't work, which honestly doesn't appear to be the intended > purpose of the new test macros. No, the current set of macros does not have the negative option. Given that you get a error from a capability check I don't think it makes much sense to test that scenario. > So do you expect to see _VER for all versions since 2.9 as well or just > a DO_TEST_CAPS_LATEST("genid"); (and "genid-auto") with adjusted result > file names instead of the DO_TEST results? Wish there were more > examples other than just disk-drive-write-cache... I honestly think that a DO_TEST_CAPS_LATEST check is enough in this case. > > The 2.9 output is different from 2.10 and 2.12, but does that matter for > this code? We don't have a comparable 2.11 output. Seems like an No the output is different because of other factors. That means that a LATEST check is okay. > excessive amount of *.args files could be generated with the only > difference being the -machine output. Although I do note that the If your output differs on the -machine bit, it will in every other release. You need to specify an explicit machine type rather than the alias. > *disk-drive-write-cache*.*.args files all have the same machine id value > while my branch has different ones for each version file. That's because > you define the machine in the XML file while mine just used 'pc'. > Leaves me wondering about the validity of what was being done /-|. Erm .... just use an explicit machine type and not the alias. > Thinking forward to when/if the next set of qemu capabilities are > created, does that mean we have to go back and create 2.12 specific > files for the current "x86_64-latest.args" file(s) since the latest will > invariably and eventually get some new stuff that will cause a > difference? Who gets that task? The first unlucky/unfortunate sole that > creates the caps_2.13.0.x86_64.xml file? Or whomever has a change > causing the difference? All this was discussed in the thread adding the testing infrastructure. The new files should be created/forked prior to adding new code which will change the output. The task is on the author of the patches that will modify the code in such way that changes the output. Note that just adding a new capability file should NOT change this testing except for cases when the new capabilities would drop a previously existing capability bit. In such case it is actually worth reviewing. > To a degree it's fortunate that genid is only in the x86_64 capabilities > I guess; otherwise, I'd be wondering again asking/pondering about > aarch64, s390x, ppc64 too... That depends on how much do you care about those. The CAPS testing infrastructure supports other arches too, I just did not create the simpler macros for it yet.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
