6:35 p.m.
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
6:35 p.m.
New subject: [libvirt] [PATCH 1/2] virsh vol-upload/download disallow negative offset
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(a)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
10:25 a.m.
New subject: [libvirt] [PATCH 1/2] virsh vol-upload/download disallow negative offset
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(a)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
11:13 a.m.
New subject: [libvirt] [PATCH 1/2] virsh vol-upload/download disallow negative offset
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(a)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
6:35 p.m.
New subject: [libvirt] [PATCH 2/2] virsh: Document bandwidth maximum more clearly
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(a)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
10:24 a.m.
New subject: [libvirt] [PATCH 2/2] virsh: Document bandwidth maximum more clearly
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(a)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
12:30 p.m.
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
3746
days inactive
3747
days old
6 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa