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

On 07/16/2014 03:18 AM, Peter Krempa wrote: >> 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 Unlimited file size is 2^63, not 2^64 (off_t is signed; so there is no way the kernel can provide you access to more than 8 exabytes. Anyone that really has 16 exabytes of data to manage [hah!] has to split it into multiple files. Treating a negative value as distance from the end of the file is always possible. Whether or not it is practical and worth coding up is another matter.

> 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. off_t is already signed; the kernel has already sacrificed half the numbers, and lseek() already has a mode to do negative offsets from the end of an arbitrarily large file, up to the 2^63 limit imposed by off_t.

> > TL;DR; > Taking negative portions of the --offset as offset from the back might > not play well with very large files. Not true. > >> >> 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. >

Another possibility is special-casing length 0 (not -1) as full file length, since it transferring 0 bytes is already a weird thing to do.

> >> 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. It's a nice quirk for anyone familiar with 2s-complement. But I can also agree that not abusing it means fewer corner cases to test.