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@xxxxxxxxxx> >> --- >> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list