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

[Peter Krempa]

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

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180425/5f357005/attachment-0001.sig>