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@xxxxxxxxxx>
> ---
> 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(¶ms[nparams++],
> + VIR_DOMAIN_BLOCK_COPY_BANDWIDTH,
> + VIR_TYPED_PARAM_ULLONG,
> + bandwidth << 20ULL) < 0)
> + goto cleanup;
> + }
> + if (granularity &&
> + virTypedParameterAssign(¶ms[nparams++],
> + VIR_DOMAIN_BLOCK_COPY_GRANULARITY,
> + VIR_TYPED_PARAM_UINT,
> + granularity) < 0)
> + goto cleanup;
> + if (buf_size &&
> + virTypedParameterAssign(¶ms[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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list