Re: [libvirt] [PATCH] blockjob: add virsh blockjob --wait

On 12.04.2012 21:59, Eric Blake wrote: > I'm tired of shell-scripting to wait for completion of a block pull, > when virsh can be taught to do the same. I couldn't quite reuse > vshWatchJob, as this is not a case of a long-running command where > a second thread must be used to probe job status (at least, not unless > I make virsh start doing blocking waits for an event to fire), but it > served as inspiration for my simpler single-threaded loop. There is > up to a half-second delay between sending SIGINT and the job being > aborted, but I didn't think it worth the complexity of a second thread > and use of poll() just to minimize that delay. > > @@ -7295,8 +7295,9 @@ repoll: > } > > GETTIMEOFDAY(&curr); > - if ( timeout && ((int)(curr.tv_sec - start.tv_sec) * 1000 + \ > - (int)(curr.tv_usec - start.tv_usec) / 1000) > timeout * 1000 ) { > + if (timeout && (((int)(curr.tv_sec - start.tv_sec) * 1000 + > + (int)(curr.tv_usec - start.tv_usec) / 1000) > > + timeout * 1000)) { > /* suspend the domain when migration timeouts. */ > vshDebug(ctl, VSH_ERR_DEBUG, "%s timeout", label); > if (timeout_func) These two chunks are rather cosmetic but I'm okay with having them in this patch not a separate one.

> + while (blocking) { > + virDomainBlockJobInfo info; > + int result = virDomainGetBlockJobInfo(dom, path, &info, 0); > + > + if (result < 0) { > + vshError(ctl, _("failed to query job for disk %s"), path); > + goto cleanup; > + } > + if (result == 0) > + break; > + > + if (verbose) > + print_job_progress(_("Block Pull"), info.end - info.cur, info.end); > + > + GETTIMEOFDAY(&curr); > + if (intCaught || (timeout && > + (((int)(curr.tv_sec - start.tv_sec) * 1000 + > + (int)(curr.tv_usec - start.tv_usec) / 1000) > > + timeout * 1000))) { > + vshDebug(ctl, VSH_ERR_DEBUG, > + intCaught ? "interrupted" : "timeout"); > + intCaught = 0; > + timeout = 0; > + quit = true; > + if (virDomainBlockJobAbort(dom, path, 0) < 0) { > + vshError(ctl, _("failed to abort job for disk %s"), path); > + goto cleanup; > + } Don't we need blocking = false; here? Otherwise we may spin another rounds and issue GetBlockJobInfo API over and over again. But on the other hand, I can imagine this is desired behavior. If BlockJobAbort succeeds and returns 0 (we are enforcing synchronous operation here) it should mean that block job has really been aborted. Therefore we may issue GetBlockJobInfo to ascertain ourselves.