[libvirt] [PATCH] virsh: improve waiting for block job readiness

[Michael Chapman]

On Mon, 4 Jan 2016, Andrea Bolognani wrote:
> On Thu, 2015-12-31 at 16:34 +1100, Michael Chapman wrote:
>> Wait for a block job event after the job has reached 100% only if
>> exactly one of the BLOCK_JOB and BLOCK_JOB_2 callbacks were successfully
>> registered.
>>  
>> If neither callback was registered, then no event will ever be received.
>> If both were registered, then any user-supplied path is guaranteed to
>> match one of the events.
>>  
>> Signed-off-by: Michael Chapman <mike at very.puzzling.org>
>> ---
>>  
>> I have found that even a 2.5 second timeout isn't always sufficiently
>> long for QEMU to flush a disk at the end of a block job.
>>  
>> I hope I've understood the code properly here, because as far as I can
>> tell the comment I'm removing in this commit isn't right. The path the
>> user supplies *has* to be either the <source file='name'/> or <target
>> dev='name'/> exactly in order for the disk to be matched, and these are
>> precisely the two strings used by the two events.
>>  
>>   tools/virsh-domain.c | 24 +++++++++++++-----------
>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>  
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index edbbc34..60de9ba 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -1942,18 +1942,20 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data)
>>               goto cleanup;
>>           }
>>   
>> -        /* since virsh can't guarantee that the path provided by the user will
>> -         * later be matched in the event we will need to keep the fallback
>> -         * approach and claim success if the block job finishes or vanishes. */
>> -        if (result == 0)
>> -            break;
>> +        /* if either event could not be registered we can't guarantee that the
>> +         * path provided by the user will be matched, so keep the fallback
>> +         * approach and claim success if the block job finishes or vanishes */
>> +        if (data->cb_id2 < 0 || data->cb_id2 < 0) {
>
> I'm not going to comment on the rest of the patch, but the condition
> above doesn't look like it would make any sense written that way...

Oops, good catch. I had meant to use cb_id and cb_id2 respectively in the 
two conditions (and in the following test as well).

>> +            if (result == 0)
>> +                break;
>>   
>> -        /* for two-phase jobs we will try to wait in the synchronized phase
>> -         * for event arrival since 100% completion doesn't necessarily mean that
>> -         * the block job has finished and can be terminated with success */
>> -        if (info.end == info.cur && --retries == 0) {
>> -            ret = VIR_DOMAIN_BLOCK_JOB_READY;
>> -            goto cleanup;
>> +            /* only wait in the synchronized phase for event arrival if
>> +             * either callback was registered */
>> +            if (info.end == info.cur &&
>> +                ((data->cb_id2 < 0 && data->cb_id2 < 0) || --retries == 0)) {
>
> ... and neither does this one.
>
> Am I missing something?
>
> Cheers.
>
>> +                ret = VIR_DOMAIN_BLOCK_JOB_READY;
>> +                goto cleanup;
>> +            }
>>           }
>>   
>>           if (data->verbose)
> -- 
> Andrea Bolognani
> Software Engineer - Virtualization Team

- Michael