On 04/30/14 06:22, Eric Blake wrote:

Commit f22b7899 stumbled across a difference between 32-bit and 64-bit platforms; on 64-bit, parsing "-1" as a long produces 0xffffffffffffffff, which does not fit in unsigned int; but on 32-bit, it parses as 0xffffffff, which DOES fit. Another patch will tweak virStrToLong_ui to behave the same across platforms, but regardless of which of the two choices it makes, the chain lookup code wants to reject negative numbers rather than treating it as large integers.

* src/util/virstoragefile.c (virStorageFileParseChainIndex): Reject negative sign.

Signed-off-by: Eric Blake <eblake@redhat.com> ---

Therefore, although this qualifies as a build-breaker fix on 32-bit, I haven't pushed it yet: I'm still working on my patch to virstring.c for uniform behavior; and that turned up the fact that virstringtest.c doesn't have coverage of integer parsing, so of course, I have to expand that test too...

src/util/virstoragefile.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index dcce1ef..5d933a4 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1525,7 +1525,9 @@ virStorageFileParseChainIndex(const char *diskTarget, if (virStringListLength(strings) != 2) goto cleanup;

- if (virStrToLong_ui(strings[1], &suffix, 10, &idx) < 0 || + /* Rule out spaces or negative sign */ + if (!c_isdigit(*strings[1]) ||

I'd rather see a wrapper that rejects negative numbers when parsing into a unsigned type rather than having it silently convert the number to a negative one. I know that there are places where this is desired, but they should be fixed to explicitly use a differnent func.

+ virStrToLong_ui(strings[1], &suffix, 10, &idx) < 0 || STRNEQ(suffix, "]")) goto cleanup;

I'm inclined to NACK this change. Peter