[libvirt] [PATCHv5 17/19] lib: Introduce flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE

[Eric Blake]

On 06/19/2014 07:59 AM, Peter Krempa wrote:
> Introduce flag for the block rebase API to allow the rebase operation to
> leave the chain relatively addressed. Also adds a virsh switch to enable
> this behavior.
> ---
>  include/libvirt/libvirt.h.in |  2 ++
>  src/libvirt.c                |  5 +++++
>  tools/virsh-domain.c         | 22 +++++++++++++++++++---
>  tools/virsh.pod              |  4 ++++
>  4 files changed, 30 insertions(+), 3 deletions(-)

Similar comments to 16/19 about being gated on qemu.git.

> +++ b/src/libvirt.c
> @@ -19716,6 +19716,11 @@ virDomainBlockPull(virDomainPtr dom, const char *disk,
>   * exists.  If the job is aborted, a new one can be started later to
>   * resume from the same point.
>   *
> + * If @flags contains VIR_DOMAIN_BLOCK_REBASE_RELATIVE, the name recorded
> + * into the overlay of the @base image as path to the new backing file
> + * will be kept relative to other images in case the backing chain was
> + * using relative names.

Quite wordy since the overlay of @base is always the active layer (given
the current limitations of blockpull); how about:

If @flags contains VIR_DOMAIN_BLOCK_REBASE_RELATIVE, the name recorded
into the active disk as the location for @base will be kept relative, if
the backing chain was using relative names.

Also needs to mention what happens if this flag is set bug @base is
omitted (silently ignored, or explicit error?)

> +++ b/tools/virsh-domain.c
> @@ -1479,10 +1479,14 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
>      case VSH_CMD_BLOCK_JOB_PULL:
>          if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0)
>              goto cleanup;
> -        if (base)
> -            ret = virDomainBlockRebase(dom, path, base, bandwidth, 0);
> -        else
> +        if (base) {
> +          if (vshCommandOptBool(cmd, "keep-relative"))
> +              flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE;

Here, you silently ignore the flag if base is omitted.  Is it worth
calling the new API when the flag is specified but base is NULL, in
order to let virsh serve as a test for what happens if the flag is set
in error?

> +
> +            ret = virDomainBlockRebase(dom, path, base, bandwidth, flags);
> +        } else {
>              ret = virDomainBlockPull(dom, path, bandwidth, 0);
> +        }

In fact, I think you want to modify flags in advance, and then do if
(base || flags) virDomainBlockRebase(); else virDomainBlockPull()

> +    {.name = "keep-relative",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("keep the backing chain relative if it was relatively "
> +                "referenced if it was before")

s/if it was before/before/

> @@ -2139,6 +2148,13 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
>      bool quit = false;
>      int abort_flags = 0;
> 
> +    if (vshCommandOptBool(cmd, "keep-relative") &&
> +        !vshCommandOptBool(cmd, "base")) {
> +        vshError(ctl, "%s", _("--keep-relative is supported only with partial "
> +                              "pull operations with --base specified"));
> +        return false;
> +    }

Again, if virsh does less validation up front, then we can ensure that
lower in the stack behaves sanely with unusual requests.  I'm not sure
this condition is worth having in virsh.

> +++ b/tools/virsh.pod

> 
> +Using the I<--keep-relative> flag will try to keep the backing chain names
> +relative (if they were relative before).

Hmm, this wording is a bit nicer compared to the sentence you added in
16/19; might be worth trying to make them similar.

Looking forward to v6.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

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