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

[Michal Prívozník]

On 06/22/2018 10:07 AM, Jiri Denemark wrote:
> 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".

I thought about it too, but then decided to take the easy way out
because this would end up being a spaghetti code in my sight. But okay,
I will rework this to:

if (blocker && agentBlocker) {
   ...
} else if (blocker) {
   ...
} else if (agentBlocker) {
   ...
} else {
   ...
}

where all four bodies are merely the same (only the error message would
change - hence the spaghetti code).

Michal