Re: [libvirt] [PATCHv2 0/3] fix virstoragetest failure on 32-bit
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
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)