Re: [Qemu-devel] [PATCH] qapi: treat all negative return of strtosz_suffix() as error

[Michael S. Tsirkin]

On Mon, Apr 28, 2014 at 12:16:02AM +0800, Amos Kong wrote:
> String "-3" will be wrongly converted to -34 by strtosz_suffix().
> strtosz_suffix_unit() only returns integer in success situation.
> 
> Signed-off-by: Amos Kong <address@hidden>


Looks correct to me.

nitpick: while not introduced by this patch,
it's cleaner to do error handling in the
if statement rather than handle success specially.
Also if (x) is nicer than if (x != 0).
So:

    if (val < 0 || *endptr) {
            error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
              "a size value representible as a non-negative int64");
            return;
    }

    *obj = val;
    processed(ov, name);


this will make this use of strtosz_suffix consistent with all
other uses.

I'm fine with changing this in  a follow-up patch though, so:

Reviewed-by: Michael S. Tsirkin <address@hidden>



> ---
>  qapi/opts-visitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 5d830a2..881e1b9 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -472,7 +472,7 @@ opts_type_size(Visitor *v, uint64_t *obj, const char 
> *name, Error **errp)
>  
>      val = strtosz_suffix(opt->str ? opt->str : "", &endptr,
>                           STRTOSZ_DEFSUFFIX_B);
> -    if (val != -1 && *endptr == '\0') {
> +    if (val >= 0 && *endptr == '\0') {
>          *obj = val;
>          processed(ov, name);
>          return;
> -- 
> 1.9.0
>