[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.
> 
> * 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]