[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 1/2] qemu: Introduce and use qemuDomainRemoveInactiveJob




On 08/15/2017 03:53 AM, Michal Privoznik wrote:
> At some places we either already have synchronous job or we just
> released it. Also, some APIs might want to use this code without
> having to release their job. Anyway, the job acquire code is
> moved out to qemuDomainRemoveInactiveJob so that
> qemuDomainRemoveInactive does just what it promises.

Feels like this is a partial thought as to what's being adjusted here. I
think essentially you're trying to state that RemoveInactiveJob is a
wrapper to RemoveInactive for paths that don't already have a non async
job. For paths with an async job, that job must first be ended before
calling/using the new InactiveJob API.

> 
> Signed-off-by: Michal Privoznik <mprivozn redhat com>
> ---
>  src/qemu/qemu_domain.c    | 36 +++++++++++++++++++++++++++---------
>  src/qemu/qemu_domain.h    |  3 +++
>  src/qemu/qemu_driver.c    | 26 +++++++++++++-------------
>  src/qemu/qemu_migration.c | 10 +++++-----
>  src/qemu/qemu_process.c   | 10 +++++-----
>  5 files changed, 53 insertions(+), 32 deletions(-)
> 

Should I assume you tested using the scenario from Martin's commit id
'b629c64e5'?

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 40608554c..2b19f841c 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5187,14 +5187,16 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver,
>      return rem.err;
>  }
>  
> -/*
> +
> +/**
> + * qemuDomainRemoveInactive:
> + *
>   * The caller must hold a lock the vm.

"hold a lock to the vm."

And this should only be called if the caller has taken a non
asynchronous job (right?)...

>   */
>  void
>  qemuDomainRemoveInactive(virQEMUDriverPtr driver,
>                           virDomainObjPtr vm)
>  {
> -    bool haveJob = true;
>      char *snapDir;
>      virQEMUDriverConfigPtr cfg;
>  
> @@ -5205,9 +5207,6 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
>  
>      cfg = virQEMUDriverGetConfig(driver);
>  
> -    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> -        haveJob = false;
> -
>      /* Remove any snapshot metadata prior to removing the domain */
>      if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0) {
>          VIR_WARN("unable to remove all snapshots for domain %s",
> @@ -5240,13 +5239,32 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
>       */
>      virObjectLock(vm);
>      virObjectUnref(cfg);
> -
> -    if (haveJob)
> -        qemuDomainObjEndJob(driver, vm);
> -
>      virObjectUnref(vm);
>  }
>  
> +
> +/**
> + * qemuDomainRemoveInactiveJob:
> + *
> + * Just like qemuDomainRemoveInactive but it tries to grab a
> + * QEMU_JOB_MODIFY before. If it doesn't succeed in grabbing the

s/before/first/
s/If it doesn't/Even though it doesn't/

> + * job the control carries with qemuDomainRemoveInactive though.

s/job the control carries with/job, continue on with the/
s/though/call/

> + */
> +void
> +qemuDomainRemoveInactiveJob(virQEMUDriverPtr driver,
> +                            virDomainObjPtr vm)
> +{
> +    bool haveJob;
> +
> +    haveJob = qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) >= 0;

Is perhaps the reason this was failing because we already had a job in
some instances? Since this is a void path on the path to destruction
failure probably won't matter, although I suppose it could be logged in
some VIR_DEBUG/WARN manner. Not a requirement, just a thought.

> +
> +    qemuDomainRemoveInactive(driver, vm);
> +
> +    if (haveJob)
> +        qemuDomainObjEndJob(driver, vm);
> +}
> +
> +
>  void
>  qemuDomainSetFakeReboot(virQEMUDriverPtr driver,
>                          virDomainObjPtr vm,
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 4c9050aff..f93b09b69 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -611,6 +611,9 @@ int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver,
>  void qemuDomainRemoveInactive(virQEMUDriverPtr driver,
>                                virDomainObjPtr vm);
>  
> +void qemuDomainRemoveInactiveJob(virQEMUDriverPtr driver,
> +                                 virDomainObjPtr vm);
> +
>  void qemuDomainSetFakeReboot(virQEMUDriverPtr driver,
>                               virDomainObjPtr vm,
>                               bool value);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e9f07c6e7..94c9c003f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1779,7 +1779,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
>      def = NULL;
>  
>      if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_START) < 0) {
> -        qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainRemoveInactiveJob(driver, vm);
>          goto cleanup;
>      }
>  
> @@ -1789,7 +1789,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
>                           start_flags) < 0) {
>          virDomainAuditStart(vm, "booted", false);
>          qemuProcessEndJob(driver, vm);
> -        qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainRemoveInactiveJob(driver, vm);
>          goto cleanup;
>      }
>  
> @@ -2259,9 +2259,9 @@ qemuDomainDestroyFlags(virDomainPtr dom,
>  
>      ret = 0;
>   endjob:
> -    qemuDomainObjEndJob(driver, vm);
>      if (ret == 0)
>          qemuDomainRemoveInactive(driver, vm);
> +    qemuDomainObjEndJob(driver, vm);
>  
>   cleanup:
>      virDomainObjEndAPI(&vm);
> @@ -3396,7 +3396,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
>      }
>      qemuDomainObjEndAsyncJob(driver, vm);
>      if (ret == 0)
> -        qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainRemoveInactiveJob(driver, vm);
>  
>   cleanup:
>      virObjectUnref(cookie);
> @@ -3916,7 +3916,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom,
>  
>      qemuDomainObjEndAsyncJob(driver, vm);
>      if (ret == 0 && flags & VIR_DUMP_CRASH)
> -        qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainRemoveInactiveJob(driver, vm);
>  
>   cleanup:
>      virDomainObjEndAPI(&vm);
> @@ -4227,7 +4227,7 @@ processGuestPanicEvent(virQEMUDriverPtr driver,
>   endjob:
>      qemuDomainObjEndAsyncJob(driver, vm);
>      if (removeInactive)
> -        qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainRemoveInactiveJob(driver, vm);
>  
>   cleanup:
>      virObjectUnref(cfg);
> @@ -4729,8 +4729,8 @@ processMonitorEOFEvent(virQEMUDriverPtr driver,
>      qemuDomainEventQueue(driver, event);
>  
>   endjob:
> -    qemuDomainObjEndJob(driver, vm);
>      qemuDomainRemoveInactive(driver, vm);
> +    qemuDomainObjEndJob(driver, vm);
>  }
>  
>  
> @@ -6680,7 +6680,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
>      VIR_FREE(xmlout);
>      virFileWrapperFdFree(wrapperFd);
>      if (vm && ret < 0)
> -        qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainRemoveInactiveJob(driver, vm);
>      virDomainObjEndAPI(&vm);
>      virNWFilterUnlockFilterUpdates();
>      return ret;
> @@ -7263,7 +7263,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
>              /* Brand new domain. Remove it */
>              VIR_INFO("Deleting domain '%s'", vm->def->name);
>              vm->persistent = 0;
> -            qemuDomainRemoveInactive(driver, vm);
> +            qemuDomainRemoveInactiveJob(driver, vm);
>          }
>          goto cleanup;
>      }
> @@ -7396,7 +7396,7 @@ qemuDomainUndefineFlags(virDomainPtr dom,
>       */
>      vm->persistent = 0;
>      if (!virDomainObjIsActive(vm))
> -        qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainRemoveInactiveJob(driver, vm);
>  
>      ret = 0;
>  
> @@ -15591,8 +15591,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>          }
>  
>          if (qemuDomainSnapshotRevertInactive(driver, vm, snap) < 0) {
> +            qemuDomainRemoveInactive(driver, vm);
>              qemuProcessEndJob(driver, vm);
> -            qemuDomainRemoveInactive(driver, vm);

Earlier in qemuDomainCreateXML we called qemuProcessEndJob first before
using qemuDomainRemoveInactiveJob, but here and in the next hunk it's a
different approach with a process job.

Shouldn't these both be InactiveJob that go after the ProcessEndJob?

>              goto cleanup;
>          }
>          if (config)
> @@ -15613,8 +15613,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>                                    start_flags);
>              virDomainAuditStart(vm, "from-snapshot", rc >= 0);
>              if (rc < 0) {
> -                qemuProcessEndJob(driver, vm);
>                  qemuDomainRemoveInactive(driver, vm);
> +                qemuProcessEndJob(driver, vm);
>                  goto cleanup;
>              }
>              detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT;
> @@ -15957,8 +15957,8 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,

There's a hunk right above here calling qemuDomainRemoveInactive when
qemuDomainObjBeginJob fails, that should be InactiveJob, right?

>      if (qemuProcessAttach(conn, driver, vm, pid,
>                            pidfile, monConfig, monJSON) < 0) {
>          monConfig = NULL;
> -        qemuDomainObjEndJob(driver, vm);
>          qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainObjEndJob(driver, vm);

Not a problem here, but mostly a mental note that I'm sure to quickly
forget!  I find it ironic that there's a qemuProcessBeginStopJob without
a qemuProcessEndStopJob, instead it's left as a "known" that the process
begin start job takes a non async job.

>          goto cleanup;
>      }
>  
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 056c051b3..b1f613430 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -2850,7 +2850,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>              virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort);
>          priv->nbdPort = 0;
>          virDomainObjRemoveTransientDef(vm);
> -        qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainRemoveInactiveJob(driver, vm);
>      }
>      qemuMigrationParamsClear(&migParams);
>      virDomainObjEndAPI(&vm);
> @@ -3291,7 +3291,7 @@ qemuMigrationConfirm(virConnectPtr conn,
>              virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm);
>              vm->persistent = 0;
>          }
> -        qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainRemoveInactiveJob(driver, vm);
>      }
>  
>   cleanup:
> @@ -4867,7 +4867,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver,
>              virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm);
>              vm->persistent = 0;
>          }
> -        qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainRemoveInactiveJob(driver, vm);
>      }
>  
>      if (orig_err) {
> @@ -4947,7 +4947,7 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver,
>      }
>  
>      if (!virDomainObjIsActive(vm))
> -        qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainRemoveInactiveJob(driver, vm);
>  
>   cleanup:
>      virDomainObjEndAPI(&vm);
> @@ -5388,7 +5388,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>  
>      qemuMigrationJobFinish(driver, vm);
>      if (!virDomainObjIsActive(vm))
> -        qemuDomainRemoveInactive(driver, vm);
> +        qemuDomainRemoveInactiveJob(driver, vm);
>  
>   cleanup:
>      VIR_FREE(jobInfo);
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index fed2bc588..b86ef3757 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6652,10 +6652,10 @@ qemuProcessAutoDestroy(virDomainObjPtr dom,
>                                       VIR_DOMAIN_EVENT_STOPPED,
>                                       VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
>  
> -    qemuDomainObjEndJob(driver, dom);
> -
>      qemuDomainRemoveInactive(driver, dom);
>  
> +    qemuDomainObjEndJob(driver, dom);
> +
>      qemuDomainEventQueue(driver, event);
>  
>   cleanup:
> @@ -6987,10 +6987,10 @@ qemuProcessReconnect(void *opaque)
>          driver->inhibitCallback(true, driver->inhibitOpaque);
>  
>   cleanup:
> -    if (jobStarted)
> -        qemuDomainObjEndJob(driver, obj);
>      if (!virDomainObjIsActive(obj))
>          qemuDomainRemoveInactive(driver, obj);
> +    if (jobStarted)
> +        qemuDomainObjEndJob(driver, obj);

There would be a chance here that RemoveInactive is called without being
in a job, perhaps this should be the rather ugly :

    if (jobStarted) {
        if (!virDomainObjIsActive())
            qemuDomainRemoveInactive
        qemuDomainObjEndJob
    } else {
        if (!virDomainObjIsActive())
            qemuDomainRemoveInactiveJob
    }


Although there's a couple questions, either you make the change or can
provide the explanation w/ regard to the design choices. So...

Reviewed-by: John Ferlan <jferlan redhat com>

John
>      virDomainObjEndAPI(&obj);
>      virObjectUnref(conn);
>      virObjectUnref(cfg);
> @@ -7065,7 +7065,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
>           */
>          qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED,
>                          QEMU_ASYNC_JOB_NONE, 0);
> -        qemuDomainRemoveInactive(src->driver, obj);
> +        qemuDomainRemoveInactiveJob(src->driver, obj);
>  
>          virDomainObjEndAPI(&obj);
>          virNWFilterUnlockFilterUpdates();
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]