On 04/30/14 06:25, Eric Blake wrote: > On 04/29/2014 02:36 AM, Michal Privoznik wrote: >> Currently, there's an issue with virStrToLong_* APIs that they turn >> "-1" into UINT_MAX. While this is not acceptable, it works on 64 bit >> architectures and doesn't work on 32 bit ones. I know that much >> cleaner solution is required, but given that we are in the freeze we >> may as well just skip the test on 32 bits. >> >> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> >> --- >> tests/virstoragetest.c | 54 ++++++++++++++++++++++++++++++++------------------ >> 1 file changed, 35 insertions(+), 19 deletions(-) > > As discussed earlier, I'm proposing an alternative patch (series). > > Part one is here: > https://www.redhat.com/archives/libvir-list/2014-April/msg01132.html > and I tested that it lets the test pass on 32-bit builds again, with a > MUCH smaller diffstat and no loss of test coverage. > > Part two is still under development (I'm in the middle of enhancing > tests/virstringtest.c to actually cover things), but here's the diff I'm > currently playing with: > > diff --git i/src/util/virstring.c w/src/util/virstring.c > index 64c7259..c646669 100644 > --- i/src/util/virstring.c > +++ w/src/util/virstring.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (C) 2012-2013 Red Hat, Inc. > + * Copyright (C) 2012-2014 Red Hat, Inc. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > @@ -221,6 +221,15 @@ virStrToLong_ui(char const *s, char **end_ptr, int > base, unsigned int *result) > > errno = 0; > val = strtoul(s, &p, base); /* exempt from syntax-check */ > + > + /* This one's tricky. We _want_ to allow "-1" as shorthand for Well we want to allow it in special cases ... I'd rather see a fix for those instances rather than having a broken-by-design wrapper that copies strange semantics of strtoul. > + * UINT_MAX, but strtoul() treats "-1" as ULONG_MAX; casting from > + * ulong back to uint changes the values only on platforms where > + * long is a larger size. */ > + if ((val & 0xffffffff00000000ULL) == 0xffffffff00000000ULL && > + memchr(s, '-', p - s)) > + val &= 0xffffffff; Uhh ... I think this makes the wrapper even worse. We might want special handlers for -1 that makes sense in some cases. If I parsed the above statement correctly any negative value passed to this function would be returned as UINT_MAX. That would convert the weird semantics of strtoul to even weirder one. > + > err = (errno || (!end_ptr && *p) || p == s || (unsigned int) val != > val); > if (end_ptr) > *end_ptr = p; > I think that our wrapper here should behave in a saner way than strtoul for most of the places in the code and we should eventually add wrappers that handle -1 as UINT_MAX and use them just in special cases. Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list