Re: [Qemu-devel] [PATCH 1/1] balloon: Ignore negative balloon values

[Markus Armbruster]

Amit Shah <address@hidden> writes:

> Negative balloon values don't make sense, ignore them.
>
> Reported-by: Mike Cao <address@hidden>
> Signed-off-by: Amit Shah <address@hidden>
> ---
> I'm not sure if error_report is the right thing to use or should a new
> qerror_report() be used.  Luiz, comments?

Since do_balloon() has been converted to qerror already, you should use
qerror_report().  Something like this should do[*]:

    qerror_report(QERR_INVALID_PARAMETER_VALUE, "target", "a size")

>  balloon.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/balloon.c b/balloon.c
> index cf9e3b2..e0ff97f 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -51,12 +51,16 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
>      return 0;
>  }
>  
> -static int qemu_balloon(ram_addr_t target)
> +static int qemu_balloon(long long target)
>  {
>      if (!balloon_event_fn) {
>          return 0;
>      }
>      trace_balloon_event(balloon_opaque, target);
> +    if (target < 0) {
> +        error_report("Ignoring negative balloon value");
> +        return -1;
> +    }
>      balloon_event_fn(balloon_opaque, target);
>      return 1;
>  }

Monitor argument type is 'M', i.e. target_long.  Caller do_balloon() it
as int64_t.  Argument passing casts it to ram_addr_t, which is unsigned.
Negative arguments get misinterpreted.

You fix it by converting to long long instead, then rejecting negative
arguments.

I think do_balloon() is a more natural place to check the argument
range.  Permits keeping qemu_balloon()'s parameter type as is.

> @@ -150,6 +154,8 @@ int do_balloon(Monitor *mon, const QDict *params,
>      if (ret == 0) {
>          qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
>          return -1;
> +    } else if (ret < 0) {
> +        return -1;
>      }
>  
>      cb(opaque, NULL);

[*] Yes, that results in a sub-par error message for humans.  Human
users are advised to appreciate that the error message was created with
proper object-oriented techniques.