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

[Amit Shah <amit.shah@redhat.com>]

On (Wed) 27 Jul 2011 [15:49:18], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > Negative balloon values don't make sense, ignore them.
> >
> > Reported-by: Mike Cao <bcao@redhat.com>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> > 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.

Ouch; that's crazy.  I don't know why I thought qdict_get_int returned
long long..  I meant to use int64_t.

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

OK, done.

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

Heh.

		Amit