Message ID | fb259790d26f17727d690dfa3bcc73fb2c2fd158.1311766038.git.amit.shah@redhat.com |
---|---|
State | New |
Headers | show |
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. 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.
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
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; } @@ -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);
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? balloon.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)