Re: [libvirt] [PATCH v2 10/11] qemu: Add VM Generation ID to qemu command line

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(a)redhat.com&gt; >> --- >> 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.

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

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

excessive amount of *.args files could be generated with the only difference being the -machine output. Although I do note that the

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

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?

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