[libvirt] [PATCH v3 2/5] virsh: expose new active commit controls

[Peter Krempa]

On 06/11/14 18:27, Eric Blake wrote:
> Add knobs to virsh to manage a 2-phase active commit of the top
> layer, similar to knobs already present on blockcopy.  While this
> code will fail until later patches actually implement the new
> knobs in the qemu driver, doing it now proves that the API is
> usable and also makes it easier for testing the qemu changes as
> they are made.
> 
> * tools/virsh-domain.c (cmdBlockCommit): Add --active, --pivot,
> and --finish options, modeled after blockcopy.

s/--finish/--keep-overlay/

> (blockJobImpl): Support --active flag.
> * tools/virsh.pod (blockcommit): Document new flags.
> (blockjob): Mention 2-phase commit interaction.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  tools/virsh-domain.c | 58 +++++++++++++++++++++++++++++++++++++++++++++-------
>  tools/virsh.pod      | 36 ++++++++++++++++++++++----------
>  2 files changed, 76 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 294b594..3db0bae 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c

> @@ -1621,7 +1638,7 @@ static const vshCmdOptDef opts_block_commit[] = {
>      {.name = "keep-relative",
>       .type = VSH_OT_BOOL,
>       .help = N_("keep the backing chain relative if it was relatively "
> -                "referenced if it was before")
> +                "referenced before")
>      },

This should go into my patch. As we discussed, this stuff has better
chances going in earlier as the relative naming patches as it already is
supported by qemu, so you should rebase this to master instead to my series.


>      {.name = NULL}
>  };
> @@ -1631,8 +1648,11 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
>  {
>      virDomainPtr dom = NULL;
>      bool ret = false;
> -    bool blocking = vshCommandOptBool(cmd, "wait");

Spurious move again?

>      bool verbose = vshCommandOptBool(cmd, "verbose");
> +    bool pivot = vshCommandOptBool(cmd, "pivot");
> +    bool finish = vshCommandOptBool(cmd, "keep-overlay");
> +    bool active = vshCommandOptBool(cmd, "active") || pivot || finish;
> +    bool blocking = vshCommandOptBool(cmd, "wait");
>      int timeout = 0;
>      struct sigaction sig_action;
>      struct sigaction old_sig_action;
> @@ -1643,7 +1663,12 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
>      bool quit = false;
>      int abort_flags = 0;
> 
> +    blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish;
>      if (blocking) {
> +        if (pivot && finish) {
> +            vshError(ctl, "%s", _("cannot mix --pivot and --keep-overlay"));
> +            return false;
> +        }

You can use VSH_EXCLUSIVE_OPTIONS("pivot", "keep-overlay") here. Also
you can bove it out of the if (blocking) block as they can't be ever
used together.

(Yes it will add a function call to get the value, but on the other hand
it will unify this piece with a majority of others).

>          if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0)
>              return false;
>          if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)

> @@ -1694,6 +1718,8 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
>          if (verbose)
>              vshPrintJobProgress(_("Block Commit"),
>                                  info.end - info.cur, info.end);

Empty line here will separate those two independent blocks visually.

> +        if (active && info.cur == info.end)
> +            break;
> 
>          GETTIMEOFDAY(&curr);
>          if (intCaught || (timeout &&
> @@ -1720,7 +1746,25 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
>          /* printf [100 %] */
>          vshPrintJobProgress(_("Block Commit"), 0, 1);
>      }

In this hunk too. Please separate the blocks.

> -    vshPrint(ctl, "\n%s", quit ? _("Commit aborted") : _("Commit complete"));
> +    if (!quit && pivot) {
> +        abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT;
> +        if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) {
> +            vshError(ctl, _("failed to pivot job for disk %s"), path);
> +            goto cleanup;
> +        }
> +    } else if (finish && !quit &&
> +               virDomainBlockJobAbort(dom, path, abort_flags) < 0) {
> +        vshError(ctl, _("failed to finish job for disk %s"), path);
> +        goto cleanup;
> +    }
> +    if (quit)
> +        vshPrint(ctl, "\n%s", _("Commit aborted"));
> +    else if (pivot)
> +        vshPrint(ctl, "\n%s", _("Successfully pivoted"));
> +    else if (!finish)
> +        vshPrint(ctl, "\n%s", _("Now in synchronized phase"));
> +    else
> +        vshPrint(ctl, "\n%s", _("Commit complete"));
> 
>      ret = true;
>   cleanup:
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 77013f5..21c29ec 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -769,8 +769,9 @@ address of virtual interface (such as I<detach-interface> or
>  I<domif-setlink>) will accept the MAC address printed by this command.
> 
>  =item B<blockcommit> I<domain> I<path> [I<bandwidth>]
> -{[I<base>] | [I<--shallow>]} [I<top>] [I<--delete>] [I<--keep-relative>]
> -[I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]]
> +[I<base>] [I<--shallow>] [I<top>] [I<--delete>] [I<--keep-relative>]
> +[I<--wait> [I<--async>] [I<--verbose>]] [I<--timeout> B<seconds>]
> +[I<--active>] [{I<--pivot> | I<--keep-overlay>}]

Again, rebase this to master please if you want to go first.


>  Reduce the length of a backing image chain, by committing changes at the
>  top of the chain (snapshot or delta files) into backing images.  By


Otherwise looks good. If you want to get this into upstream prior to my
series then please rebase and repost this patch.

If you are about to wait until the relative-backing stuff goes in, then
ACK with the nits fixed.

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140612/4886feb4/attachment-0001.sig>