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