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@xxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
