[libvirt] [PATCH 3/5] qemu_domain: Introduce qemuDomainObjBeginJobInstant

[John Ferlan]

FWIW: While I'm not the best at naming things, it would seem to me that
this should be named "Nowait" rather than "Instant".

On 06/07/2018 07:59 AM, Michal Privoznik wrote:
> The aim of this API is to allow caller do best effort. Some

s/allow caller/allow the caller/

s/do best effort/to avoid waiting for a domain job if the domain is
running something else that could take a long time./

> functions of ours can work even when acquiring job fails (e.g.

s/of ours//

s/job/the job/

> qemuConnectGetAllDomainStats()). But what they can't bear is
> delay if they have to wait up to 30 seconds for each domain.

s/./that is processing some other job./

> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_domain.c | 30 +++++++++++++++++++++++++-----
>  src/qemu/qemu_domain.h |  4 ++++
>  2 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5273ab56ac..9657573342 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6340,11 +6340,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job)
>   * @obj: domain object
>   * @job: qemuDomainJob to start
>   * @asyncJob: qemuDomainAsyncJob to start
> + * @instant: don't wait trying to acquire @job

Prefer "nowait" (or some camelCase version of that)

>   *
>   * Acquires job over domain object which must be locked before
>   * calling. If there's already a job running waits up to
>   * QEMU_JOB_WAIT_TIME after which the functions fails reporting
> - * an error.
> + * an error unless @instant is set.
> + * If @instant is true this function tries to acquire job and if
> + * it fails returns immediately without waiting. No error is

s/it fails returns/it fails, then it returns/

> + * reported in this case.

Hmm... no error reported - meaning if ret = -1 and use this flag, it's
up to the caller to generate the error...  For this series, perhaps
fine, but someone else using this, perhaps not.

>   *
>   * Returns: 0 on success,
>   *         -2 if unable to start job because of timeout or
> @@ -6355,7 +6359,8 @@ static int ATTRIBUTE_NONNULL(1)
>  qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>                                virDomainObjPtr obj,
>                                qemuDomainJob job,
> -                              qemuDomainAsyncJob asyncJob)
> +                              qemuDomainAsyncJob asyncJob,
> +                              bool instant)
>  {
>      qemuDomainObjPrivatePtr priv = obj->privateData;
>      unsigned long long now;
> @@ -6395,12 +6400,18 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>      }
>  
>      while (!nested && !qemuDomainNestedJobAllowed(priv, job)) {
> +        if (instant)
> +            goto cleanup;
> +

If async is not supported for this (as I believe seen in the new API),
then is this path possible?

>          VIR_DEBUG("Waiting for async job (vm=%p name=%s)", obj, obj->def->name);
>          if (virCondWaitUntil(&priv->job.asyncCond, &obj->parent.lock, then) < 0)
>              goto error;
>      }
>  
>      while (priv->job.active) {
> +        if (instant)
> +            goto cleanup;
> +
>          VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj, obj->def->name);
>          if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0)
>              goto error;
> @@ -6517,7 +6528,7 @@ int qemuDomainObjBeginJob(virQEMUDriverPtr driver,
>                            qemuDomainJob job)
>  {
>      if (qemuDomainObjBeginJobInternal(driver, obj, job,
> -                                      QEMU_ASYNC_JOB_NONE) < 0)
> +                                      QEMU_ASYNC_JOB_NONE, false) < 0)
>          return -1;
>      else
>          return 0;
> @@ -6532,7 +6543,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver,
>      qemuDomainObjPrivatePtr priv;
>  
>      if (qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC,
> -                                      asyncJob) < 0)
> +                                      asyncJob, false) < 0)
>          return -1;
>  
>      priv = obj->privateData;
> @@ -6561,9 +6572,18 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
>  
>      return qemuDomainObjBeginJobInternal(driver, obj,
>                                           QEMU_JOB_ASYNC_NESTED,
> -                                         QEMU_ASYNC_JOB_NONE);
> +                                         QEMU_ASYNC_JOB_NONE,
> +                                         false);
>  }
>  
> +int
> +qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver,
> +                             virDomainObjPtr obj,
> +                             qemuDomainJob job)
> +{
> +    return qemuDomainObjBeginJobInternal(driver, obj, job,
> +                                         QEMU_ASYNC_JOB_NONE, true);
                                            ^^^^^^^^^^^^^^^^^^^^
Doesn't this mean async jobs are not supported.

John

> +}
>  
>  /*
>   * obj must be locked and have a reference before calling
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index f17157b951..4b63c00dff 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -512,6 +512,10 @@ int qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
>                                  virDomainObjPtr obj,
>                                  qemuDomainAsyncJob asyncJob)
>      ATTRIBUTE_RETURN_CHECK;
> +int qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver,
> +                                 virDomainObjPtr obj,
> +                                 qemuDomainJob job)
> +    ATTRIBUTE_RETURN_CHECK;
>  
>  void qemuDomainObjEndJob(virQEMUDriverPtr driver,
>                           virDomainObjPtr obj);
>