[libvirt] [PATCH 0/2] virsh: negative numbers for specific commands
Following up to the recently restarted discussion: http://www.redhat.com/archives/libvir-list/2014-July/msg00686.html regarding negative values for certain virsh commands - these patches will document the "feature" of using a negative value to indicate the largest value *and* for the vol-{upload|download} change 'offset' to not accept a negative value. John Ferlan (2): virsh vol-upload/download disallow negative offset virsh: Document bandwidth maximum more clearly tools/virsh-volume.c | 6 +++--- tools/virsh.pod | 33 +++++++++++++++++++++++++++------ 2 files changed, 30 insertions(+), 9 deletions(-) -- 1.9.3
https://bugzilla.redhat.com/show_bug.cgi?id=1087104 Commit id 'c6212539' explicitly allowed a negative value to be used for offset and length as a shorthand for the largest value after commit id 'f18c02ec' modified virStrToLong_ui() to essentially disallow a negative value. However, allowing a negative value for offset ONLY worked if the negative value was -1 since the eventual lseek() does allow a -1 to mean the end of the file. Providing other negative values resulted in errors such as: $ virsh vol-download --pool default qcow3-vol2 /home/vm-images/raw \ --offset -2 --length -1000 error: cannot download from volume qcow3-vol2 error: Unable to seek /home/vm-images/qcow3-vol2 to 18446744073709551614: Invalid argument $ Thus, it seems unreasonable to expect or allow a negative value for offset since the only benefit is to lseek() to the end of the file and then only take advantage of how the OS would handle such a seek. For the purposes of upload or download of volume data, that seems to be a no-op. Therefore, disallow a negative value for offset. Additionally, modify the man page for vol-upload and vol-download to provide more details regarding the valid values for both offset and length. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-volume.c | 6 +++--- tools/virsh.pod | 10 ++++++++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 724a86b..b528928 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -787,13 +787,13 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd) unsigned long long offset = 0, length = 0; bool created = false; - if (vshCommandOptULongLongWrap(cmd, "offset", &offset) < 0) { - vshError(ctl, _("Unable to parse integer")); + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) { + vshError(ctl, _("Unable to parse offset value")); return false; } if (vshCommandOptULongLongWrap(cmd, "length", &length) < 0) { - vshError(ctl, _("Unable to parse integer")); + vshError(ctl, _("Unable to parse length value")); return false; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 1a2b01f..e02ff5d 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2961,7 +2961,10 @@ is in. I<vol-name-or-key-or-path> is the name or key or path of the volume where the file will be uploaded. I<--offset> is the position in the storage volume at which to start writing -the data. I<--length> is an upper bound of the amount of data to be uploaded. +the data. The value must be 0 or larger. I<--length> is an upper bound +of the amount of data to be uploaded. A negative value is interpreted +as an unsigned long long value to essentially include everything from +the offset to the end of the volume. An error will occur if the I<local-file> is greater than the specified length. =item B<vol-download> [I<--pool> I<pool-or-uuid>] [I<--offset> I<bytes>] @@ -2972,7 +2975,10 @@ I<--pool> I<pool-or-uuid> is the name or UUID of the storage pool the volume is in. I<vol-name-or-key-or-path> is the name or key or path of the volume to download. I<--offset> is the position in the storage volume at which to start reading -the data. I<--length> is an upper bound of the amount of data to be downloaded. +the data. The value must be 0 or larger. I<--length> is an upper bound of +the amount of data to be downloaded. A negative value is interpreted as +an unsigned long long value to essentially include everything from the +offset to the end of the volume. =item B<vol-wipe> [I<--pool> I<pool-or-uuid>] [I<--algorithm> I<algorithm>] I<vol-name-or-key-or-path> -- 1.9.3
On 07/17/14 01:35, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1087104
Commit id 'c6212539' explicitly allowed a negative value to be used for offset and length as a shorthand for the largest value after commit id 'f18c02ec' modified virStrToLong_ui() to essentially disallow a negative value.
However, allowing a negative value for offset ONLY worked if the negative value was -1 since the eventual lseek() does allow a -1 to mean the end of the file. Providing other negative values resulted in errors such as:
$ virsh vol-download --pool default qcow3-vol2 /home/vm-images/raw \ --offset -2 --length -1000 error: cannot download from volume qcow3-vol2 error: Unable to seek /home/vm-images/qcow3-vol2 to 18446744073709551614: Invalid argument
$
Thus, it seems unreasonable to expect or allow a negative value for offset since the only benefit is to lseek() to the end of the file and then only take advantage of how the OS would handle such a seek. For the purposes of upload or download of volume data, that seems to be a no-op. Therefore, disallow a negative value for offset.
Additionally, modify the man page for vol-upload and vol-download to provide more details regarding the valid values for both offset and length.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-volume.c | 6 +++--- tools/virsh.pod | 10 ++++++++-- 2 files changed, 11 insertions(+), 5 deletions(-)
ACK, although I'd prefer Eric stating his opinion on this too. Peter
On 07/17/2014 09:25 AM, Peter Krempa wrote:
On 07/17/14 01:35, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1087104
Commit id 'c6212539' explicitly allowed a negative value to be used for offset and length as a shorthand for the largest value after commit id 'f18c02ec' modified virStrToLong_ui() to essentially disallow a negative value.
However, allowing a negative value for offset ONLY worked if the negative value was -1 since the eventual lseek() does allow a -1 to mean the end of the file. Providing other negative values resulted in errors such as:
lseek to -1 is not necessarily the end of the file. But I'm not sure what a better wording of this part of the commit message would be.
$ virsh vol-download --pool default qcow3-vol2 /home/vm-images/raw \ --offset -2 --length -1000 error: cannot download from volume qcow3-vol2 error: Unable to seek /home/vm-images/qcow3-vol2 to 18446744073709551614: Invalid argument
$
Thus, it seems unreasonable to expect or allow a negative value for offset since the only benefit is to lseek() to the end of the file and then only take advantage of how the OS would handle such a seek. For the purposes of upload or download of volume data, that seems to be a no-op. Therefore, disallow a negative value for offset.
Additionally, modify the man page for vol-upload and vol-download to provide more details regarding the valid values for both offset and length.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-volume.c | 6 +++--- tools/virsh.pod | 10 ++++++++-- 2 files changed, 11 insertions(+), 5 deletions(-)
ACK, although I'd prefer Eric stating his opinion on this too.
At any rate, the code and documentation of this patch are fine with me - keeping the negative length as shorthand to catch the rest of the file is fine. And if we ever want to add negative offset as distance from the end of the file, we can still do that in the future; but disabling it for now is fine. I think the ACK stands. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Commit id '0e2d7305' modified the code to allow a negative value to be supplied for the bandwidth argument of the various block virsh commands and the migrate-setspeed; however, it failed to update the man page to describe the "feature" whereby a very large value could be interpreted by the hypervisor to mean maximum value allowed. Although initially designed to handle a -1 value, the reality is just about any negative value could be provided and essentially perform the same feature. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh.pod | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index e02ff5d..849ae31 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -865,7 +865,10 @@ 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. +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. =item B<blockcopy> I<domain> I<path> I<dest> [I<bandwidth>] [I<--shallow>] [I<--reuse-external>] [I<--raw>] [I<--wait> [I<--async>] [I<--verbose>]] @@ -908,7 +911,10 @@ 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. +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. =item B<blockpull> I<domain> I<path> [I<bandwidth>] [I<base>] [I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]] @@ -939,7 +945,10 @@ 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. +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. =item B<blkdeviotune> I<domain> I<device> [[I<--config>] [I<--live>] | [I<--current>]] @@ -995,6 +1004,9 @@ commit job be pivoted over to the new image. If I<--info> is specified, the active job information on the specified disk will be printed. 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. =item B<blockresize> I<domain> I<path> I<size> @@ -1390,7 +1402,10 @@ obtained from domjobinfo. =item B<migrate-setspeed> I<domain> I<bandwidth> Set the maximum migration bandwidth (in MiB/s) for a domain which is being -migrated to another host. +migrated to another host. I<bandwidth> is interpreted as an unsigned long +long value. Specifying a negative value results in an essentially unlimited +value being provided to the hypervisor. The hypervisor can choose whether to +reject the value or convert it to the maximum value allowed. =item B<migrate-getspeed> I<domain> -- 1.9.3
On 07/17/14 01:35, John Ferlan wrote:
Commit id '0e2d7305' modified the code to allow a negative value to be supplied for the bandwidth argument of the various block virsh commands
Technically that commit didn't allow it. It was allowed before that commit kept it in that same state.
and the migrate-setspeed; however, it failed to update the man page to describe the "feature" whereby a very large value could be interpreted by the hypervisor to mean maximum value allowed. Although initially designed to handle a -1 value, the reality is just about any negative value could be provided and essentially perform the same feature.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh.pod | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
ACK, I like the wording where you specify that the hypervisor may reject that. Peter
On 07/16/2014 07:35 PM, John Ferlan wrote:
Following up to the recently restarted discussion:
http://www.redhat.com/archives/libvir-list/2014-July/msg00686.html
regarding negative values for certain virsh commands - these patches will document the "feature" of using a negative value to indicate the largest value *and* for the vol-{upload|download} change 'offset' to not accept a negative value.
John Ferlan (2): virsh vol-upload/download disallow negative offset virsh: Document bandwidth maximum more clearly
tools/virsh-volume.c | 6 +++--- tools/virsh.pod | 33 +++++++++++++++++++++++++++------ 2 files changed, 30 insertions(+), 9 deletions(-)
Thanks - pushed. John
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa