3:36 a.m.
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(a)redhat.com>
---
tests/virstoragetest.c | 54 ++++++++++++++++++++++++++++++++------------------
1 file changed, 35 insertions(+), 19 deletions(-)
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 018469a..9e81782 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -23,19 +23,26 @@
#include <stdlib.h>
#include "testutils.h"
-#include "vircommand.h"
-#include "virerror.h"
-#include "virfile.h"
-#include "virlog.h"
-#include "virstoragefile.h"
-#include "virstring.h"
-#include "dirname.h"
-#define VIR_FROM_THIS VIR_FROM_NONE
+/*
+ * XXX An awful workaround as this test doesn't run
+ * cleanly on 32 bit architectures.
+ */
+#ifdef __LP64__
+
+# include "vircommand.h"
+# include "virerror.h"
+# include "virfile.h"
+# include "virlog.h"
+# include "virstoragefile.h"
+# include "virstring.h"
+# include "dirname.h"
+
+# define VIR_FROM_THIS VIR_FROM_NONE
VIR_LOG_INIT("tests.storagetest");
-#define datadir abs_builddir "/virstoragedata"
+# define datadir abs_builddir "/virstoragedata"
/* This test creates the following files, all in datadir:
@@ -239,14 +246,14 @@ testPrepImages(void)
goto cleanup;
}
-#ifdef HAVE_SYMLINK
+# ifdef HAVE_SYMLINK
/* Create some symlinks in a sub-directory. */
if (symlink("../qcow2", datadir "/sub/link1") < 0 ||
symlink("../wrap", datadir "/sub/link2") < 0) {
fprintf(stderr, "unable to create symlink");
goto cleanup;
}
-#endif
+# endif
ret = 0;
cleanup:
@@ -513,7 +520,7 @@ mymain(void)
if ((ret = testPrepImages()) != 0)
return ret;
-#define TEST_ONE_CHAIN(id, start, format, flags, ...) \
+# define TEST_ONE_CHAIN(id, start, format, flags, ...) \
do { \
size_t i; \
memset(&data, 0, sizeof(data)); \
@@ -528,10 +535,10 @@ mymain(void)
ret = -1; \
} while (0)
-#define VIR_FLATTEN_2(...) __VA_ARGS__
-#define VIR_FLATTEN_1(_1) VIR_FLATTEN_2 _1
+# define VIR_FLATTEN_2(...) __VA_ARGS__
+# define VIR_FLATTEN_1(_1) VIR_FLATTEN_2 _1
-#define TEST_CHAIN(id, relstart, absstart, format, chain1, flags1, \
+# define TEST_CHAIN(id, relstart, absstart, format, chain1, flags1, \
chain2, flags2, chain3, flags3, chain4, flags4) \
do { \
TEST_ONE_CHAIN(#id "a", relstart, format, flags1, \
@@ -788,7 +795,7 @@ mymain(void)
(&dir), EXP_PASS,
(&dir), ALLOW_PROBE | EXP_PASS);
-#ifdef HAVE_SYMLINK
+# ifdef HAVE_SYMLINK
/* Rewrite qcow2 and wrap file to use backing names relative to a
* symlink from a different directory */
virCommandFree(cmd);
@@ -838,7 +845,7 @@ mymain(void)
(&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS,
(&link2, &link1, &raw), EXP_PASS,
(&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS);
-#endif
+# endif
/* Rewrite qcow2 to be a self-referential loop */
virCommandFree(cmd);
@@ -894,7 +901,7 @@ mymain(void)
goto cleanup;
}
-#define TEST_LOOKUP_TARGET(id, target, name, index, result, meta, parent) \
+# define TEST_LOOKUP_TARGET(id, target, name, index, result, meta, parent) \
do { \
struct testLookupData data2 = { chain, target, name, index, \
result, meta, parent, }; \
@@ -902,7 +909,7 @@ mymain(void)
testStorageLookup, &data2) < 0) \
ret = -1; \
} while (0)
-#define TEST_LOOKUP(id, name, result, meta, parent) \
+# define TEST_LOOKUP(id, name, result, meta, parent) \
TEST_LOOKUP_TARGET(id, NULL, name, 0, result, meta, parent)
TEST_LOOKUP(0, "bogus", NULL, NULL, NULL);
@@ -1012,3 +1019,12 @@ mymain(void)
}
VIRT_TEST_MAIN(mymain)
+
+#else
+
+int main(void)
+{
+ return EXIT_AM_SKIP;
+}
+
+#endif /* __LP64___ */
--
1.9.0
3:45 a.m.
On Tue, Apr 29, 2014 at 10:36:51 +0200, 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.
I think a workaround using strtoull instead of stroul in virStrToLong_ui
would be better, even though I don't like that solution either. Your
workaround disables the check but keeps the functionality which is
tested here broken.
Jirka
3:57 a.m.
On Tue, Apr 29, 2014 at 10:45:56AM +0200, Jiri Denemark wrote:
On Tue, Apr 29, 2014 at 10:36:51 +0200, 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.
I think a workaround using strtoull instead of stroul in virStrToLong_ui
would be better, even though I don't like that solution either. Your
workaround disables the check but keeps the functionality which is
tested here broken.
Moreover, Pavel found out that strtoul should set errno to ERANGE in
case of the problem described here. The only problem is that we *are*
checking for errno there and it just passes :(
Martin
4 a.m.
On Tue, Apr 29, 2014 at 10:57:50 +0200, Martin Kletzander wrote:
On Tue, Apr 29, 2014 at 10:45:56AM +0200, Jiri Denemark wrote:
>On Tue, Apr 29, 2014 at 10:36:51 +0200, 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.
>
>I think a workaround using strtoull instead of stroul in virStrToLong_ui
>would be better, even though I don't like that solution either. Your
>workaround disables the check but keeps the functionality which is
>tested here broken.
>
Moreover, Pavel found out that strtoul should set errno to ERANGE in
case of the problem described here. The only problem is that we *are*
checking for errno there and it just passes :(
But only in case the non-negative value does not fit in. If it fits, no
error is reported even though negative number is parsed. The API is just
insane.
Jirka
8:11 a.m.
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(a)redhat.com>
---
tests/virstoragetest.c | 54 ++++++++++++++++++++++++++++++++------------------
1 file changed, 35 insertions(+), 19 deletions(-)
NACK - this is papering over the problem. The point of freeze is to fix
the root cause of the problem, not paper over it. I'll look into this
today, and propose an alternative.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:25 p.m.
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(a)redhat.com>
---
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
+ * 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;
+
err = (errno || (!end_ptr && *p) || p == s || (unsigned int) val !=
val);
if (end_ptr)
*end_ptr = p;
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:47 a.m.
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(a)redhat.com>
> ---
> 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
7:33 a.m.
On 04/30/2014 03:47 AM, Peter Krempa wrote:
> 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.
Okay, so I'll definitely prepare two variants of each of the three
"parse to unsigned" wrappers - one that allows '-', the other rejects
it. Callers can choose which of the two they want.
> + * 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.
No. The intent is as follows. Parsing to 'long' or 'unsigned long'
behaves identically to '[unsigned] long long' on 64-bit platforms and to
'[unsigned] int' on 32-bit platforms (really, that just means using
strtol/strtoul as-is). Parsing to '[unsigned] long long' behaves
identically across platforms (really, just using strtoll/strtoull
as-is). Parsing to '[unsigned] int' behaves as though using a 32-bit
strtol (on 32-bit platforms, it is easy; on 64-bit platforms we have to
do some fudge work). Whether we reject '-' on the unsigned parse is
determined by using a new function, so we have three variants (signed,
unsigned with sign, unsigned without sign) for three types (int, long,
long long).
Remember, the POSIX requirements are that strtol() accepts numbers in
the range LONG_MIN to LONG_MAX, while strtoul() accepts numbers in the
range LONG_MIN to ULONG_MAX (yes, strtoul has a larger range).
The interesting boundary cases for int are:
Anything between 0 and 2147483647 parses as itself, for all three int
variants
Anything between 2147483648 and 4294967295 parses as itself for both
unsigned int variants, but rejected as out-of-bounds for signed int
(this is the behavior required of 32-bit strtol/strtoul)
Anything at 4294967296 or larger is rejected as out-of-bounds for all
three int variants
-0 is parsed as 0 for signed int and unsigned int with sign, and is
rejected as out-of-bounds for unsigned int without sign
Anything between -1 and -2147483647 parses as itself for signed int,
parses as the negation (4294967295 through 2147483649) for unsigned int
with sign, and is rejected as out-of-bounds for unsigned int without sign
-2147483648 parses as itself for signed int, parses as 2147483648 for
unsigned int with sign, and is rejected as out-of-bounds for unsigned
int without sign
Anything between -2147483649 and -4294967295 is rejected as out of
bounds for signed int and unsigned int without sign, and is parsed as
the negation (2147483647 through 1) for unsigned int with sign
Anything at -4294967296 or beyond is rejected as out-of-bounds for all
three int variants
That's why I'm working up the test suite before pushing anything; you'll
see exactly in the testsuite what I _plan_ to accept, and I may have to
tweak the code to match that.
> +
> 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.
Okay, I'm using that as guidance on the patches I'm preparing today.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:52 a.m.
On 04/30/2014 06:33 AM, Eric Blake wrote:
Remember, the POSIX requirements are that strtol() accepts numbers in
the range LONG_MIN to LONG_MAX, while strtoul() accepts numbers in the
range LONG_MIN to ULONG_MAX (yes, strtoul has a larger range).
Correction: strtoul() is required to accept things in the range
-ULONG_MAX to ULONG_MAX (which is larger than the range LONG_MIN to
ULONG_MAX).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3888
days inactive
3889
days old
8 comments
5 participants
participants (5)
-
Eric Blake -
Jiri Denemark -
Martin Kletzander -
Michal Privoznik -
Peter Krempa