Treating -1 as the maximum bandwidth of block jobs is not very
practical, given the restrictions on overflow between 32-bit
vs. 64-bit long, as well as conversions between MiB/s and bytes/s.
We already document that 0 means unlimited, so the only reason to
keep negative support is for back-compat.
Meanwhile, it is a good idea to allow users to input in scales
other than MiB/s. This patch just rounds back up to MiB/s, but by
using a common helper, a later patch can then determine when a
particular input scale will be better for using flags with the
corresponding API call.
* tools/virsh-domain.c (blockJobBandwidth): New function.
(blockJobImpl, cmdBlockCopy): Use it.
* tools/virsh.pod (blockjob, blockcopy, blockpull, blockcommit):
Document this.
Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
---
tools/virsh-domain.c | 73 ++++++++++++++++++++++++++++++++++++++++++----------
tools/virsh.pod | 42 ++++++++++++++++++------------
2 files changed, 84 insertions(+), 31 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index cc1e554..594647a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1460,6 +1460,9 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
goto cleanup;
}
+
+/* Code common between blockpull, blockcommit, blockcopy, blockjob */
+
typedef enum {
VSH_CMD_BLOCK_JOB_ABORT,
VSH_CMD_BLOCK_JOB_SPEED,
@@ -1467,6 +1470,56 @@ typedef enum {
VSH_CMD_BLOCK_JOB_COMMIT,
} vshCmdBlockJobMode;
+
+/* Parse a block job bandwidth parameter. Returns -1 on error with
+ * message printed, 0 if no bandwidth needed, and 1 if *bandwidth is
+ * set to non-zero. */
+static int
+blockJobBandwidth(vshControl *ctl, const vshCmd *cmd,
+ unsigned long *bandwidth)
+{
+ const char *value = NULL;
+ int ret = vshCommandOptString(cmd, "bandwidth", &value);
+ unsigned long long speed;
+
+ *bandwidth = 0;
+ if (ret <= 0)
+ return ret;
+
+ /* For back-compat, accept -1 as meaning unlimited, by converting
+ * negative values to 0 (and forbid wraparound back to positive).
+ * Alas, we have to parse the raw string ourselves, instead of
+ * using vshCommandOptUL. If only we had never allowed wraparound
+ * on bandwidth. */
+ if (strchr(value, '-')) {
+ unsigned long tmp;
+
+ if (vshCommandOptULWrap(cmd, "bandwidth", &tmp) < 0 ||
+ (long) tmp >= 0) {
+ vshError(ctl, _("could not parse bandwidth '%s'"), value);
+ return -1;
+ }
+ return 0;
+ }
+
+ /* Read a number in bytes/s, but with default unit of MiB/s if no
+ * unit was specified. */
+ if (vshCommandOptScaledInt(cmd, "bandwidth", &speed, 1024 * 1024,
+ (sizeof(*bandwidth) < sizeof(speed) ?
+ ULONG_MAX * 1024 * 1024 : ULLONG_MAX)) < 0) {
+ vshError(ctl, _("could not parse bandwidth '%s'"), value);
+ return -1;
+ }
+
+ /* Now scale it to MiB/s for the caller. What a pain. */
+ speed = VIR_DIV_UP(speed, 1024 * 1024);
+
+ /* We already checked that narrowing the type will fit. */
+ *bandwidth = speed;
+ return !!speed;
+}
+
+
static bool
blockJobImpl(vshControl *ctl, const vshCmd *cmd,
vshCmdBlockJobMode mode, virDomainPtr *pdom)
@@ -1485,10 +1538,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
goto cleanup;
- if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) {
- vshError(ctl, "%s", _("bandwidth must be a number"));
+ if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0)
goto cleanup;
- }
switch (mode) {
case VSH_CMD_BLOCK_JOB_ABORT:
@@ -1610,7 +1661,7 @@ static const vshCmdOptDef opts_block_commit[] = {
},
{.name = "bandwidth",
.type = VSH_OT_DATA,
- .help = N_("bandwidth limit in MiB/s")
+ .help = N_("bandwidth limit, as scaled integer (default MiB/s)")
},
{.name = "base",
.type = VSH_OT_DATA,
@@ -1826,7 +1877,7 @@ static const vshCmdOptDef opts_block_copy[] = {
},
{.name = "bandwidth",
.type = VSH_OT_DATA,
- .help = N_("bandwidth limit in MiB/s")
+ .help = N_("bandwidth limit, as scaled integer (default MiB/s)")
},
{.name = "shallow",
.type = VSH_OT_BOOL,
@@ -1959,14 +2010,8 @@ 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"));
+ if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0)
goto cleanup;
- }
if (vshCommandOptUInt(cmd, "granularity", &granularity) < 0) {
vshError(ctl, "%s", _("granularity must be a number"));
goto cleanup;
@@ -2182,7 +2227,7 @@ static const vshCmdOptDef opts_block_job[] = {
},
{.name = "bandwidth",
.type = VSH_OT_DATA,
- .help = N_("set the Bandwidth limit in MiB/s")
+ .help = N_("bandwidth limit, as scaled integer (default MiB/s)")
},
{.name = NULL}
};
@@ -2341,7 +2386,7 @@ static const vshCmdOptDef opts_block_pull[] = {
},
{.name = "bandwidth",
.type = VSH_OT_DATA,
- .help = N_("bandwidth limit in MiB/s")
+ .help = N_("bandwidth limit, as scaled integer (default MiB/s)")
},
{.name = "base",
.type = VSH_OT_DATA,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 60ee515..7aba571 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -892,11 +892,12 @@ I<path> specifies fully-qualified path of the disk; it corresponds
to a unique target name (<target dev='name'/>) or source file (<source
file='name'/>) for one of the disk devices attached to I<domain> (see
also B<domblklist> for listing these names).
-I<bandwidth> specifies copying bandwidth limit in MiB/s, although for
-qemu, it may be non-zero only for an online domain. 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.
+I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to
+MiB/s if there is no suffix. It can be used to set a bandwidth limit for
+the commit job. For backward compatibility, a negative value is treated
+the same as 0 (unlimited); the actual upper limit accepted by the command
+may depend on the word size of both client and server, or by further limits
+of the hypervisor.
=item B<blockcopy> I<domain> I<path> { I<dest> [I<format>] [I<--blockdev>]
| I<xml> } [I<--shallow>] [I<--reuse-external>] [I<bandwidth>]
@@ -943,10 +944,13 @@ as fast as possible, otherwise the command may continue to block a little
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 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
+I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to
+MiB/s if there is no suffix. It can be used to set a bandwidth limit for
+the copy job. For backward compatibility, a negative value is treated
+the same as 0 (unlimited); the actual upper limit accepted by the command
+may depend on the word size of both client and server, or by further limits
+of the hypervisor.
+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>
@@ -983,10 +987,12 @@ I<path> specifies fully-qualified path of the disk; it corresponds
to a unique target name (<target dev='name'/>) or source file (<source
file='name'/>) for one of the disk devices attached to I<domain> (see
also B<domblklist> for listing these names).
-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.
+I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to
+MiB/s if there is no suffix. It can be used to set a bandwidth limit for
+the pull job. For backward compatibility, a negative value is treated
+the same as 0 (unlimited); the actual upper limit accepted by the command
+may depend on the word size of both client and server, or by further limits
+of the hypervisor.
=item B<blkdeviotune> I<domain> I<device>
[[I<--config>] [I<--live>] | [I<--current>]]
@@ -1050,10 +1056,12 @@ not supply bytes/s resolution; when omitting the flag, raw output is
listed in MiB/s and human-readable output automatically selects the
best resolution supported by the server.
-I<bandwidth> can be used to set bandwidth limit for the active job.
-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.
+I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to
+MiB/s if there is no suffix. It can be used to set bandwidth limit for
+the active job. For backward compatibility, a negative value is treated
+the same as 0 (unlimited); the actual upper limit accepted by the command
+may depend on the word size of both client and server, or by further limits
+of the hypervisor.
=item B<blockresize> I<domain> I<path> I<size>
--
1.9.3
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list