Re: [PATCH] conf: handle 'vda[-1]' uniformly

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> ---
> 
> 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

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]