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

Re: [libvirt] [PATCH v3 12/18] blockcopy: expose new API in virsh



On 08/31/14 06:02, Eric Blake wrote:
> Expose the new power of virDomainBlockCopy through virsh (well,
> all but the finer-grained bandwidth, as that is its own can of
> worms for a later patch).  Continue to use the older API where
> possible, for maximum compatibility.
> 
> The command now requires either --dest (with optional --format
> and --blockdev), to directly describe the file destination, or
> --xml, to name a file that contains an XML description such as:
> 
> <disk type='network'>
>   <driver type='raw'/>
>   <source protocol='gluster' name='vol1/img'>
>     <host name='red'/>
>   </source>
> </disk>
> 
> Non-zero option parameters are converted into virTypedParameters,
> and if anything requires the new API, the command can synthesize
> appropriate XML even if the --dest option was used instead of --xml.
> 
> The existing --raw flag remains for back-compat, but the preferred
> spelling is now --format=raw, since the new API now allows us
> to specify all formats rather than just a boolean raw to suppress
> probing.
> 
> I hope I did justice in describing the effects of granularity and
> buf-size on how they get passed through to qemu.
> 
> * tools/virsh-domain.c (cmdBlockCopy): Add new options --xml,
> --granularity, --buf-size, --format. Make --raw an alias for
> --format=raw. Call new API if new parameters are in use.
> * tools/virsh.pod (blockcopy): Document new options.
> 
> Signed-off-by: Eric Blake <eblake redhat com>
> ---
>  tools/virsh-domain.c | 145 +++++++++++++++++++++++++++++++++++++++++++++------
>  tools/virsh.pod      |  45 +++++++++-------
>  2 files changed, 156 insertions(+), 34 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index b92b2eb..16d7f05 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1893,6 +1912,23 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
>      const char *path = NULL;
>      bool quit = false;
>      int abort_flags = 0;
> +    const char *xml = NULL;
> +    char *xmlstr = NULL;
> +    virTypedParameterPtr params = NULL;
> +    int nparams = 0;
> +
> +    if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
> +        return false;
> +    if (vshCommandOptString(cmd, "dest", &dest) < 0)
> +        return false;
> +    if (vshCommandOptString(cmd, "xml", &xml) < 0)
> +        return false;
> +    if (vshCommandOptString(cmd, "format", &format) < 0)
> +        return false;
> +
> +    VSH_EXCLUSIVE_OPTIONS_VAR(dest, xml);

These were designed to work on booleans, but pointers are fortunately
sharing the semantics :)

> +    VSH_EXCLUSIVE_OPTIONS_VAR(format, xml);
> +    VSH_EXCLUSIVE_OPTIONS_VAR(blockdev, xml);
> 
>      if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
>          return false;
> @@ -1926,24 +1962,101 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
>      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>          goto cleanup;
> 
> +    /* XXX: Parse bandwidth as scaled input, rather than forcing
> +     * MiB/s, and either reject negative input or treat it as 0 rather
> +     * than trying to guess which value will work well across both
> +     * APIs with their different sizes and scales.  */
>      if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) {
>          vshError(ctl, "%s", _("bandwidth must be a number"));
>          goto cleanup;
>      }
> +    if (vshCommandOptUInt(cmd, "granularity", &granularity) < 0) {
> +        vshError(ctl, "%s", _("granularity must be a number"));
> +        goto cleanup;
> +    }
> +    if (vshCommandOptULongLong(cmd, "buf-size", &buf_size) < 0) {
> +        vshError(ctl, "%s", _("buf-size must be a number"));
> +        goto cleanup;
> +    }
> 
> +    if (xml) {
> +        if (virFileReadAll(xml, 8192, &xmlstr) < 0) {
> +            vshReportError(ctl);
> +            goto cleanup;
> +        }
> +    } else if (!dest) {
> +        vshError(ctl, "%s", _("need either --dest or --xml"));
> +        goto cleanup;
> +    }
> +
> +    /* Exploit that VIR_DOMAIN_BLOCK_REBASE_* and
> +     * VIR_DOMAIN_BLOCK_COPY_* flags have the same values.  */
>      if (vshCommandOptBool(cmd, "shallow"))
>          flags |= VIR_DOMAIN_BLOCK_REBASE_SHALLOW;
>      if (vshCommandOptBool(cmd, "reuse-external"))
>          flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT;
> -    if (vshCommandOptBool(cmd, "raw"))
> -        flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW;
> -    if (vshCommandOptBool(cmd, "blockdev"))
> -        flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV;
> -    if (vshCommandOptStringReq(ctl, cmd, "dest", &dest) < 0)
> -        goto cleanup;
> -
> -    if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0)
> -        goto cleanup;
> +
> +    if (granularity || buf_size || (format && STRNEQ(format, "raw")) || xml) {
> +        /* New API */
> +        if (bandwidth || granularity || buf_size) {
> +            params = vshCalloc(ctl, 3, sizeof(*params));
> +            if (bandwidth) {
> +                /* bandwidth is ulong MiB/s, but the typed parameter is
> +                 * ullong bytes/s; make sure we don't overflow */
> +                if (bandwidth > LLONG_MAX >> 20) {
> +                    virReportError(VIR_ERR_OVERFLOW,
> +                                   _("bandwidth must be less than %llu"),
> +                                   LLONG_MAX >> 20);
> +                }
> +                if (virTypedParameterAssign(&params[nparams++],
> +                                            VIR_DOMAIN_BLOCK_COPY_BANDWIDTH,
> +                                            VIR_TYPED_PARAM_ULLONG,
> +                                            bandwidth << 20ULL) < 0)
> +                    goto cleanup;
> +            }
> +            if (granularity &&
> +                virTypedParameterAssign(&params[nparams++],
> +                                        VIR_DOMAIN_BLOCK_COPY_GRANULARITY,
> +                                        VIR_TYPED_PARAM_UINT,
> +                                        granularity) < 0)
> +                goto cleanup;
> +            if (buf_size &&
> +                virTypedParameterAssign(&params[nparams++],
> +                                        VIR_DOMAIN_BLOCK_COPY_BUF_SIZE,
> +                                        VIR_TYPED_PARAM_ULLONG,
> +                                        buf_size) < 0)
> +                goto cleanup;
> +        }
> +
> +        if (!xmlstr) {
> +            virBuffer buf = VIR_BUFFER_INITIALIZER;
> +            virBufferEscapeString(&buf, "<disk type='%s'>\n",
> +                                  blockdev ? "block" : "file");

This one doesn't really need escaping

> +            virBufferAdjustIndent(&buf, 2);
> +            virBufferEscapeString(&buf, "<source %s",
> +                                  blockdev ? "dev" : "file");

and this either

> +            virBufferEscapeString(&buf, "='%s'/>\n", dest);
> +            virBufferEscapeString(&buf, "<driver type='%s'/>\n", format);
> +            virBufferAdjustIndent(&buf, -2);
> +            virBufferAddLit(&buf, "</disk>\n");
> +            if (virBufferCheckError(&buf) < 0)
> +                goto cleanup;
> +            xmlstr = virBufferContentAndReset(&buf);
> +        }
> +
> +        if (virDomainBlockCopy(dom, path, xmlstr, params, nparams, flags) < 0)
> +            goto cleanup;
> +    } else {
> +        /* Old API */
> +        flags |= VIR_DOMAIN_BLOCK_REBASE_COPY;
> +        if (vshCommandOptBool(cmd, "blockdev"))
> +            flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV;
> +        if (STREQ_NULLABLE(format, "raw"))
> +            flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW;
> +
> +        if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0)
> +            goto cleanup;
> +    }
> 
>      if (!blocking) {
>          vshPrint(ctl, "%s", _("Block Copy started"));
> @@ -2014,6 +2127,8 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
> 
>      ret = true;
>   cleanup:
> +    VIR_FREE(xmlstr);
> +    virTypedParamsFree(params, nparams);
>      if (dom)
>          virDomainFree(dom);
>      if (blocking)
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 2923fd9..c3a76dc 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -898,30 +898,31 @@ value is interpreted as an unsigned long long value or essentially
>  unlimited. The hypervisor can choose whether to reject the value or
>  convert it to the maximum value allowed.
> 
> -=item B<blockcopy> I<domain> I<path> I<dest> [I<bandwidth>] [I<--shallow>]
> -[I<--reuse-external>] [I<--raw>] [I<--blockdev>]
> -[I<--wait> [I<--async>] [I<--verbose>]]
> -[{I<--pivot> | I<--finish>}] [I<--timeout> B<seconds>]
> +=item B<blockcopy> I<domain> I<path> { I<dest> [I<format>] [I<--blockdev>]
> +| I<xml> } [I<--shallow>] [I<--reuse-external>] [I<bandwidth>]
> +[I<--wait> [I<--async>] [I<--verbose>]] [{I<--pivot> | I<--finish>}]
> +[I<--timeout> B<seconds>] [I<granularity>] [I<buf-size>]
> 
> -Copy a disk backing image chain to I<dest>. By default, this command
> -flattens the entire chain; but if I<--shallow> is specified, the copy
> -shares the backing chain.
> +Copy a disk backing image chain to a destination.  Either I<dest> as
> +the destination file name, or I<xml> as the name of an XML file containing
> +a top-level <disk> element describing the destination, must be present.
> +Additionally, if I<dest> is given, I<format> should be specified to declare
> +the format of the destination (if I<format> is omitted, then libvirt
> +will reuse the format of the source, or with I<--reuse-external> will
> +be forced to probe the destination format, which could be a potential
> +security hole).  The command supports I<--raw> as a boolean flag synonym for
> +I<--format=raw>.  When using I<dest>, the destination is treated as a regular
> +file unless I<--blockdev> is used to signal that it is a block device. By
> +default, this command flattens the entire chain; but if I<--shallow> is
> +specified, the copy shares the backing chain.
> 
> -If I<--reuse-external> is specified, then I<dest> must exist and have
> +If I<--reuse-external> is specified, then the destination must exist and have
>  sufficient space to hold the copy. If I<--shallow> is used in
>  conjunction with I<--reuse-external> then the pre-created image must have
>  guest visible contents identical to guest visible contents of the backing
>  file of the original image. This may be used to modify the backing file
>  names on the destination.
> 
> -The format of the destination is determined by the first match in the
> -following list: if I<--raw> is specified, it will be raw; if
> -I<--reuse-external> is specified, the existing destination is probed
> -for a format; and in all other cases, the destination format will
> -match the source format.  The destination is treated as a regular
> -file unless I<--blockdev> is used to signal that it is a block
> -device.
> -
>  By default, the copy job runs in the background, and consists of two
>  phases.  Initially, the job must copy all data from the source, and
>  during this phase, the job can only be canceled to revert back to the
> @@ -943,9 +944,15 @@ while longer until the job has actually cancelled.
> 
>  I<path> specifies fully-qualified path of the disk.
>  I<bandwidth> specifies copying bandwidth limit in MiB/s. Specifying a negative
> -value is interpreted as an unsigned long long value or essentially
> -unlimited. The hypervisor can choose whether to reject the value or
> -convert it to the maximum value allowed.
> +value is interpreted as an unsigned long long value that might be essentially
> +unlimited, but more likely would overflow; it is safer to use 0 for that
> +purpose.  Specifying I<granularity> allows fine-tuning of the granularity that
> +will be copied when a dirty region is detected; larger values trigger less
> +I/O overhead but may end up copying more data overall (the default value is
> +usually correct); this value must be a power of two.  Specifying I<buf-size>
> +will control how much data can be simultaneously in-flight during the copy;
> +larger values use more memory but may allow faster completion (the default
> +value is usually correct).
> 
>  =item B<blockpull> I<domain> I<path> [I<bandwidth>] [I<base>]
>  [I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]]
> 

ACK as is, all comments are pure cosmetic.

Peter

Attachment: signature.asc
Description: OpenPGP digital signature


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