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

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(a)very.puzzling.org&gt; ---   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) {

+            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)) {

+                ret = VIR_DOMAIN_BLOCK_JOB_READY; +                goto cleanup; +            }           }              if (data->verbose)