[libvirt] [PATCH 2/2] qemuDomainObjBeginJobInternal: Report agent job in error message

[Jiri Denemark]

On Wed, Jun 20, 2018 at 14:32:04 +0200, Michal Privoznik wrote:
> If a thread is unable to acquire a job (e.g. because of timeout)
> an error is reported and the error message contains reference to
> the other thread holding the job. Well, the error message should
> report agent job too as it is yet another source of possible
> failure.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_domain.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 827597d5f3..4331b95917 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6420,6 +6420,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>      bool async = job == QEMU_JOB_ASYNC;
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>      const char *blocker = NULL;
> +    const char *agentBlocker = NULL;
>      int ret = -1;
>      unsigned long long duration = 0;
>      unsigned long long agentDuration = 0;
> @@ -6549,16 +6550,21 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>               priv->job.apiFlags,
>               duration / 1000, agentDuration / 1000, asyncDuration / 1000);
>  
> -    if (nested || qemuDomainNestedJobAllowed(priv, job))
> -        blocker = priv->job.ownerAPI;
> -    else
> -        blocker = priv->job.asyncOwnerAPI;
> +    if (job) {
> +        if (nested || qemuDomainNestedJobAllowed(priv, job))
> +            blocker = priv->job.ownerAPI;
> +        else
> +            blocker = priv->job.asyncOwnerAPI;
> +    }
> +
> +    if (agentJob)
> +        agentBlocker = priv->job.agentOwnerAPI;
>  
>      if (errno == ETIMEDOUT) {
> -        if (blocker) {
> +        if (blocker || agentBlocker) {
>              virReportError(VIR_ERR_OPERATION_TIMEOUT,
> -                           _("cannot acquire state change lock (held by %s)"),
> -                           blocker);
> +                           _("cannot acquire state change lock (held by %s %s)"),
> +                           NULLSTR(blocker), NULLSTR(agentBlocker));

Since this is an error message reported to the user I think we should
make it a little bit more user friendly. It would be nice to distinguish
state change lock and agent lock and only print the relevant blocker
(rather than both when one of them is NULL). And when both blockers are
reported, we should separate them better, e.g., "%s and %s".

Jirka