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.

diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index 724a86bc66bc916cbe54258802c5c13245c4d9f3..b528928741bf22968228e5e0b835553107661b4f 100644 (file)
--- 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 1a2b01fb2c94668d01b3529f53ef3cd72b91763e..e02ff5daed10612f13ff67c4a8f00f20dd9f772c 100644 (file)
--- 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>