On 05/25/2018 04:24 AM, Michal Privoznik wrote:
> 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@xxxxxxxxxx>
>> ---
>> 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.
>
I dunno, it's just before building the command line in qemuProcessLaunch
- IDC if it moves, but there may have been a request from some earlier
review to make it be called earlier.
Still moving it has implications on the entire logic. Essentially it's a
"common" place before we generate the command line to make sure if GenID
is desired/wanted that we have the capability and then can if we need to
for this path change the genid value. That need is based on starting
the domain after specific events - there is a list in the spec:
http://go.microsoft.com/fwlink/?LinkId=260709
and you'd have to read the replies from the v2 review series:
https://www.redhat.com/archives/libvir-list/2018-April/msg02234.html
in particular lots of discussion in patch 6
>> +
>> + /* 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).
>
Right, strange - that got chopped off somewhere either during a rebase
or cut-paste-copy from a previous review... I even posted/pushed the
patch that changed all of them to use the < 0 syntax. Fixed this.
>> + 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.
>
That was the whole point of adding the flag - only a handful of explicit
paths require changing the vmgen id. Those are controlled by calls to
qemuProcessStart which eventually calls ProcessLaunch where we not only
check the capability but the flag in order to force a change to the
GenID based on whether we're in a path that requires a change.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
