On 05/25/2018 12:58 PM, John Ferlan wrote:
>
>
> 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
Okay, so leave it here.
>
>>> +
>>> + /* 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.
>
Okay. For some reason having a special flag looked as an overkill to me,
but I can live with that.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
