[libvirt] [PATCH 1/2] virsh vol-upload/download disallow negative offset

[Eric Blake]

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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140717/07dd29aa/attachment-0001.sig>