[libvirt] [REPOSTv2 PATCH v3 3/6] qemu: Alter VM Generation ID for specific startup/launch transitions

[Michal Privoznik]

On 05/17/2018 02:42 PM, John Ferlan wrote:
> Before we generate the command line for qemu, if the domain about to
> be launched desires to utilize the VM Generation ID functionality, then
> handle both the regenerating the GUID value for backup recovery (restore
> operation) and the startup after snapshot as both require a new GUID to
> be generated to allow the guest operating system to recognize the VM
> is re-executing something that has already executed before.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/qemu/qemu_driver.c  | 17 ++++++++++++++---
>  src/qemu/qemu_process.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>  src/qemu/qemu_process.h |  1 +
>  3 files changed, 59 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b697838070..7e968f475d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6554,7 +6554,8 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
>      if (qemuProcessStart(conn, driver, vm, cookie ? cookie->cpu : NULL,
>                           asyncJob, "stdio", *fd, path, NULL,
>                           VIR_NETDEV_VPORT_PROFILE_OP_RESTORE,
> -                         VIR_QEMU_PROCESS_START_PAUSED) == 0)
> +                         VIR_QEMU_PROCESS_START_PAUSED |
> +                         VIR_QEMU_PROCESS_START_GEN_VMID) == 0)
>          restored = true;
>  
>      if (intermediatefd != -1) {
> @@ -15827,6 +15828,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>                      compatible = qemuDomainCheckABIStability(driver, vm, config);
>                  }
>  
> +                /* If using VM GenID, there is no way currently to change
> +                 * the genid for the running guest, so set an error and
> +                 * mark as incompatible. */
> +                if (compatible && config->genidRequested) {
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                                   _("domain genid update requires restart"));
> +                    compatible = false;
> +                }
> +
>                  if (!compatible) {
>                      virErrorPtr err = virGetLastError();
>  
> @@ -15907,7 +15917,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>                                    cookie ? cookie->cpu : NULL,
>                                    QEMU_ASYNC_JOB_START, NULL, -1, NULL, snap,
>                                    VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
> -                                  VIR_QEMU_PROCESS_START_PAUSED);
> +                                  VIR_QEMU_PROCESS_START_PAUSED |
> +                                  VIR_QEMU_PROCESS_START_GEN_VMID);
>              virDomainAuditStart(vm, "from-snapshot", rc >= 0);
>              detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT;
>              event = virDomainEventLifecycleNewFromObj(vm,
> @@ -15993,7 +16004,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>                       VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
>              /* Flush first event, now do transition 2 or 3 */
>              bool paused = (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) != 0;
> -            unsigned int start_flags = 0;
> +            unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID;
>  
>              start_flags |= paused ? VIR_QEMU_PROCESS_START_PAUSED : 0;
>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 5b73a61962..f9bba69f80 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6072,6 +6072,43 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
>  }
>  
>  
> +/**
> + * qemuProcessGenID:
> + * @vm: Pointer to domain object
> + * @flags: qemuProcessStartFlags
> + *
> + * If this domain is requesting to use genid
> + */
> +static int
> +qemuProcessGenID(virDomainObjPtr vm,
> +                 unsigned int flags)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> +    if (!vm->def->genidRequested)
> +        return 0;
> +
> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_VMGENID)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                      _("this QEMU does not support the 'genid' capability"));
> +        return -1;
> +    }

This looks misplaced to me. Firstly, if domain is required to generate
new vmid this function should do that. Secondly, this check is missing
when generating command line.

> +
> +    /* If we are coming from a path where we must provide a new gen id
> +     * value regardless of whether it was previously generated or provided,
> +     * then generate a new GUID value before we build the command line. */
> +    if (flags & VIR_QEMU_PROCESS_START_GEN_VMID) {
> +        if (virUUIDGenerate(vm->def->genid)) {

Please do explicit comparison here. It might bite us in the future when
we want virUUIDGenerate to return a positive value (which would still be
success).

> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("failed to regenerate genid"));
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
>  /**
>   * qemuProcessLaunch:
>   *
> @@ -6124,7 +6161,8 @@ qemuProcessLaunch(virConnectPtr conn,
>      virCheckFlags(VIR_QEMU_PROCESS_START_COLD |
>                    VIR_QEMU_PROCESS_START_PAUSED |
>                    VIR_QEMU_PROCESS_START_AUTODESTROY |
> -                  VIR_QEMU_PROCESS_START_NEW, -1);
> +                  VIR_QEMU_PROCESS_START_NEW |
> +                  VIR_QEMU_PROCESS_START_GEN_VMID, -1);
>  
>      cfg = virQEMUDriverGetConfig(driver);
>  
> @@ -6148,6 +6186,9 @@ qemuProcessLaunch(virConnectPtr conn,
>          goto cleanup;
>      logfile = qemuDomainLogContextGetWriteFD(logCtxt);
>  
> +    if (qemuProcessGenID(vm, flags) < 0)
> +        goto cleanup;
> +
>      VIR_DEBUG("Building emulator command line");
>      if (!(cmd = qemuBuildCommandLine(driver,
>                                       qemuDomainLogContextGetManager(logCtxt),
> @@ -6513,7 +6554,8 @@ qemuProcessStart(virConnectPtr conn,
>  
>      virCheckFlagsGoto(VIR_QEMU_PROCESS_START_COLD |
>                        VIR_QEMU_PROCESS_START_PAUSED |
> -                      VIR_QEMU_PROCESS_START_AUTODESTROY, cleanup);
> +                      VIR_QEMU_PROCESS_START_AUTODESTROY |
> +                      VIR_QEMU_PROCESS_START_GEN_VMID, cleanup);
>  
>      if (!migrateFrom && !snapshot)
>          flags |= VIR_QEMU_PROCESS_START_NEW;
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index a0e34b1c85..5098eacfe8 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -80,6 +80,7 @@ typedef enum {
>      VIR_QEMU_PROCESS_START_AUTODESTROY  = 1 << 2,
>      VIR_QEMU_PROCESS_START_PRETEND      = 1 << 3,
>      VIR_QEMU_PROCESS_START_NEW          = 1 << 4, /* internal, new VM is starting */
> +    VIR_QEMU_PROCESS_START_GEN_VMID     = 1 << 5, /* Generate a new VMID */

I wonder if we can do without this flag. Basically, only a handful of
paths require VMID regeneration. But I haven't read older versions so
maybe you had what I'm suggesting and were told to switch to flag.

Michal