Re: [libvirt] [PATCHv2 0/3] fix virstoragetest failure on 32-bit
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.
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...
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.
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?
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