[libvirt] [PATCHv3 1/2] virsh: Reject negative numbers in vshCommandOptUInt

[Peter Krempa]

On 07/15/14 00:51, John Ferlan wrote:
> 
> Bumping again - I found a related bz in my backlog...
> 
> On 05/29/2014 01:15 PM, Eric Blake wrote:
>> On 05/29/2014 05:54 AM, Peter Krempa wrote:
>>> Use virStrToLong_uip instead of virStrToLong_ui to reject negative
>>> numbers in the helper. None of the callers expects the wraparound
>>> "feature" for negative numbers.
>>
>> I had to audit all callers, and found the following (fortunately the
>> list is fairly small):
>>
> 
> <snip>
> 
>>
>> vol-upload, vol-download [offset, length]: Can't a negative offset be
>> treated as offset from the end of the file? And doesn't -1 as implying
>> unlimited length make sense?  Here, allowing -1 might make sense
>>
> 
> BZ - https://bugzilla.redhat.com/show_bug.cgi?id=1087104 has a different
> take on whether negative values should be allowed.  In particular:
> 
> "
> Additional Info:
> In manual page:
> vol-download [--pool pool-or-uuid] [--offset bytes] [--length bytes]
> 
>  --offset is the position in the storage volume
>            at which to start reading the data. --length is an upper bound of
>            the amount of data to be downloaded.
> 
> It is said that offset is the position in the storage volume at which to
> start reading the data, so I think the value of offset should be no
> smaller than 0, option length as well.
> "
> 
> 
> 
> So - do we "adjust" the man page to indicate that using a -1 is "OK" and
> what it would do?  Probably similar type action for the changes made
> (commit id's 0e2d73051 && c62125395)?
> 
> Or does a negative "really" make sense for offset?  Sure -1 makes sense
> and works because 'lseek()' allows it, but other negative numbers I just
> tried get an error:

Well it might make sense semantically but it certainly isn't coded up.
We'd need to wrap the number sensibly according to the length of the
file so that it would mean the offset from the end of the file as Eric
suggested.

Also we might add the docs about how the offset is wrapped to virsh but
reject those numbers in the API.

This said, I'm not a big fan of the negative offset as with the
theoretically unlimited file sizes (2^64bytes) you might want to have
the full number space of the value we are passing as the offset
available for very long offsets. Granted, that will not happen for a
while so we might want to sacrifice half of the numbers for negative
offsets, but we should've gone with signed types in that case.

TL;DR;
Taking negative portions of the --offset as offset from the back might
not play well with very large files.

> 
> 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
> 
> 
> Not even sure what a negative length file is... Is that the definition
> of a sparse file? Is the suggestion that we'd be downloading (or

Eric thought of lenght of -1 as full file lenght.


> uploading) from end of file backwards some number of bytes?  Not quite
> sure that's what's happening as the negative value is turned positive
> and it seems means "everything".
> 
> So while, yes, -1 for both makes sense as a sort of pseudonym for
> maximum - other values don't, but how does one go about distinguishing
> that? (eg, that a -1 was supplied and it's OK, although other negative
> values are not).

We should have designed the APIs with signed types if we wanted to take
signed numbers. I still think of parsing -1 into a unsigned type as a
quirk that we shouldn't abuse very much.


> 
> John
> 
> 

Peter


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