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.
>
> * tools/virsh.c (cmdBlockPull): Add new options to wait for
> completion.
> (blockJobImpl): Add argument.
> (cmdBlockJob): Adjust caller.
> * tools/virsh.pod (blockjob): Document new mode.
> ---
> tools/virsh.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> tools/virsh.pod | 11 ++++-
> 2 files changed, 119 insertions(+), 14 deletions(-)
>
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 8ef57e0..c54add9 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -7274,11 +7274,11 @@ repoll:
> if (pollfd.revents & POLLIN &&
> saferead(pipe_fd, &retchar, sizeof(retchar)) > 0 &&
> retchar == '0') {
> - if (verbose) {
> - /* print [100 %] */
> - print_job_progress(label, 0, 1);
> - }
> - break;
> + if (verbose) {
> + /* print [100 %] */
> + print_job_progress(label, 0, 1);
> + }
> + break;
> }
> goto cleanup;
> }
> @@ -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.
> @@ -7519,7 +7520,8 @@ typedef enum {
>
> static int
> blockJobImpl(vshControl *ctl, const vshCmd *cmd,
> - virDomainBlockJobInfoPtr info, int mode)
> + virDomainBlockJobInfoPtr info, int mode,
> + virDomainPtr *pdom)
> {
> virDomainPtr dom = NULL;
> const char *name, *path;
> @@ -7560,7 +7562,9 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
> }
>
> cleanup:
> - if (dom)
> + if (pdom && ret == 0)
> + *pdom = dom;
> + else if (dom)
> virDomainFree(dom);
> return ret;
> }
> @@ -7580,15 +7584,109 @@ static const vshCmdOptDef opts_block_pull[] = {
> {"bandwidth", VSH_OT_DATA, VSH_OFLAG_NONE, N_("Bandwidth limit in MB/s")},
> {"base", VSH_OT_DATA, VSH_OFLAG_NONE,
> N_("path of backing file in chain for a partial pull")},
> + {"wait", VSH_OT_BOOL, 0, N_("wait for job to finish")},
> + {"verbose", VSH_OT_BOOL, 0, N_("with --wait, display the progress")},
> + {"timeout", VSH_OT_INT, VSH_OFLAG_NONE,
> + N_("with --wait, abort if pull exceeds timeout (in seconds)")},
> {NULL, 0, 0, NULL}
> };
>
> static bool
> cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
> {
> - if (blockJobImpl(ctl, cmd, NULL, VSH_CMD_BLOCK_JOB_PULL) != 0)
> + virDomainPtr dom = NULL;
> + bool ret = false;
> + bool blocking = vshCommandOptBool(cmd, "wait");
> + bool verbose = vshCommandOptBool(cmd, "verbose");
> + int timeout = 0;
> + struct sigaction sig_action;
> + struct sigaction old_sig_action;
> + sigset_t sigmask;
> + struct timeval start;
> + struct timeval curr;
> + const char *path = NULL;
> + bool quit = false;
> +
> + if (blocking) {
> + if (vshCommandOptInt(cmd, "timeout", &timeout) > 0) {
> + if (timeout < 1) {
> + vshError(ctl, "%s", _("migrate: Invalid timeout"));
> + return false;
> + }
> +
> + /* Ensure that we can multiply by 1000 without overflowing. */
> + if (timeout > INT_MAX / 1000) {
> + vshError(ctl, "%s", _("migrate: Timeout is too big"));
> + return false;
> + }
> + }
> + if (vshCommandOptString(cmd, "path", &path) < 0)
> + return false;
> +
> + sigemptyset(&sigmask);
> + sigaddset(&sigmask, SIGINT);
> +
> + intCaught = 0;
> + sig_action.sa_sigaction = vshCatchInt;
> + sigemptyset(&sig_action.sa_mask);
> + sigaction(SIGINT, &sig_action, &old_sig_action);
> +
> + GETTIMEOFDAY(&start);
> + } else if (verbose || vshCommandOptBool(cmd, "timeout")) {
> + vshError(ctl, "%s", _("blocking control options require --wait"));
> return false;
> - return true;
> + }
> +
> + if (blockJobImpl(ctl, cmd, NULL, VSH_CMD_BLOCK_JOB_PULL,
> + blocking ? &dom : NULL) != 0)
> + goto cleanup;
> +
> + 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.
> + }
> +
> + usleep(500 * 1000);
> + }
> +
> + if (verbose && !quit) {
> + /* printf [100 %] */
> + print_job_progress(_("Block Pull"), 0, 1);
> + }
> + vshPrint(ctl, "\n%s\n", quit ? _("Pull aborted") : _("Pull complete"));
> +
> + ret = true;
> +cleanup:
> + if (dom)
> + virDomainFree(dom);
> + if (blocking)
> + sigaction(SIGINT, &old_sig_action, NULL);
> + return ret;
> }
>
> /*
> @@ -7639,7 +7737,7 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
> else
> mode = VSH_CMD_BLOCK_JOB_INFO;
>
> - ret = blockJobImpl(ctl, cmd, &info, mode);
> + ret = blockJobImpl(ctl, cmd, &info, mode, NULL);
> if (ret < 0)
> return false;
>
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 5f3d9b1..fbc34f1 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -640,6 +640,7 @@ address of virtual interface (such as I<detach-interface> or
> I<domif-setlink>) will accept the MAC address printed by this command.
>
> =item B<blockpull> I<domain> I<path> [I<bandwidth>] [I<base>]
> +[I<--wait> [I<--verbose>] [I<--timeout> B<seconds>]]
>
> Populate a disk from its backing image chain. By default, this command
> flattens the entire chain; but if I<base> is specified, containing the
> @@ -647,8 +648,14 @@ name of one of the backing files in the chain, then that file becomes
> the new backing file and only the intermediate portion of the chain is
> pulled. Once all requested data from the backing image chain has been
> pulled, the disk no longer depends on that portion of the backing chain.
> -It pulls data for the entire disk in the background, the process of the
> -operation can be checked with B<blockjob>.
> +
> +By default, this command returns as soon as possible, and data for
> +the entire disk is pulled in the background; the process of the
> +operation can be checked with B<blockjob>. However, if I<--wait> is
> +specified, then this command will block until the operation completes,
> +or cancel the operation if the optional I<timeout> in seconds elapses
> +or SIGINT is sent (usually with C<Ctrl-C>). Using I<--verbose> along
> +with I<--wait> will produce periodic status updates.
>
> I<path> specifies fully-qualified path of the disk; it corresponds
> to a unique target name (<target dev='name'/>) or source file (<source
ACK
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list