[libvirt] [PATCHv2 0/3] fix virstoragetest failure on 32-bit

[Eric Blake]

On 05/01/2014 02:41 PM, John Ferlan wrote:
> 
> 
> On 04/30/2014 10:27 PM, Eric Blake wrote:
>> v1 was here, but it got nacked for being ugly:
>> https://www.redhat.com/archives/libvir-list/2014-April/msg01132.html
>>
>> so in this version, I improved virstring first, then used the
>> new clean function.
>>
>> This is a build-breaker fix, so it deserves to be in 1.2.4;
>> but it is large enough that it needs a review to go in.
>>
>> Eric Blake (3):
>>   util: fix uint parsing on 64-bit platforms
>>   util: new stricter unsigned int parsing
>>   storage: reject negative indices
>>
>>  src/libvirt_private.syms  |   3 +
>>  src/util/virstoragefile.c |   2 +-
>>  src/util/virstring.c      |  98 +++++++++++++++++++++--
>>  src/util/virstring.h      |  14 +++-
>>  tests/virstringtest.c     | 200 +++++++++++++++++++++++++++++++++++++++++++++-
>>  5 files changed, 307 insertions(+), 10 deletions(-)
>>
> In short ACK...  Since I believe this resolves the issue seen.

Thanks; series pushed.

> 
> Oh the games we have to play because 32 (and 16) bit computing still
> exists... I've looked at the changes... Ran them thru coverity...
> Things seem OK to me.  Although we may have some work to do to check
> users of the existing API's...

I'm glad it passed Coverity, with no warnings about dead code due to an
impossible comparison (to be honest, I was a bit worried that it might
have complained about 'long > UINT_MAX' always being false and thus dead
code on 32-bit platforms; even though it can obviously be true on a
64-bit platform).

> 
> Having to "read minds" about what someone "really" meant when they used
> -1 in a CLI/API can be dangerous. Was using -1 meant to produce an error
> or was it meant to be a shortcut for a largest value or was it a result
> of using a 64 bit value in order to manipulate a 32 bit number? If you
> know the underlying code doesn't accept (or want) negative numbers, then
> -1 would be bad.  My favorite is of course finding those places where
> someone uses ~0ULL (or some variant).
> 
> Having to ensure/describe what each datum is for each API/CLI reference
> could be a time consuming excursion through a lot of code in order to
> determine and validate what "should" and/or "could" be passed.

Yeah, there's probably other places in libvirt that _should_ use the new
_uip variants to force a positive parse; vs. those that have already
accepted a -1 parse and where back-compat says we can't go back.  I only
focused on getting the framework in (now the caller can choose which of
the two semantics they want) and fixing the one place where the
testsuite proved that we wanted the positive-only semantics.

> 
> The other side of the coin is - what happens on display if say someone
> indicates -1 on input (for whatever reason) - does the display side then
> display the maximum whatever value or does it (or could/should it)
> display the -1?

Yeah, we've fought that battle as well.  For example, we WANT the XML to
express '(uid_t)-1' as -1, and NOT as the maximum number; but have had
mishaps in the past where we exposed the maximum number so now the code
has hacks to support both spellings.  I won't be surprised if the battle
resurfaces in the future; but at least now we have a framework to make
it easier.

> 
> One example that comes to mind (because of a recent changes (c4206d7 and
> dff3ad0) that affected virt-test/tp-libvirt results) is from
> migrate-setspeed where the maximum value allowed is INT64_MAX (although
> it's not documented as that in the man page)...  Although the recent
> changes don't allow passing -1 thru the virsh command - when they did -
> the output on a getspeed would be the 1844*1615 value (UINT64_MAX). The
> bz associated indicated use of -1 by the tester in order to force use of
> the maximum value, which is where it caught my attention as being
> somewhat related to this particular change...
> 
> John
> 
> 

-- 
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/20140501/e9327f92/attachment-0001.sig>