On Fri, Jul 06, 2018 at 08:25:57AM +0200, Markus Armbruster wrote:
> Peter Xu <pet...@redhat.com> writes:
>
> > On Thu, Jul 05, 2018 at 06:42:01PM +0200, Marc-André Lureau wrote:
> >> qmp_error_response() will free the given error. Fix double-free in
> >> later qmp_request_free().
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > Reviewed-by: Peter Xu <pet...@redhat.com>
> >
> > And not related to current patch...
> >
> >> ---
> >> monitor.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index 3c9c97b73f..7af1f18d13 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -4186,6 +4186,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >> } else {
> >> assert(req_obj->err);
> >> rsp = qmp_error_response(req_obj->err);
> >> + req_obj->err = NULL;
> >> monitor_qmp_respond(req_obj->mon, rsp, NULL);
> >
> > ... here not sure whether we should just pass in req_obj->id instead
> > of NULL, or maybe we can do some more assertions like:
> >
> > diff --git a/monitor.c b/monitor.c
> > index 9eb9f06599..04d2c50f4e 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4215,10 +4215,12 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >
> > mon = req_obj->mon;
> > if (req_obj->req) {
> > + assert(!req_obj->err);
>
> Makes sense.
>
> > trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?:
> > "");
> > monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
> > } else {
> > assert(req_obj->err);
> > + assert(!req_obj->id);
>
> I'd simply pass req_obj->id to monitor_qmp_respond(). Yes, it'll always
> be null, but the code would do the right thing if that should ever
> change.
Agreed.
>
> > rsp = qmp_error_response(req_obj->err);
> > monitor_qmp_respond(mon, rsp, NULL);
> > qobject_unref(rsp);
> >
> > Thanks,
>
> Perhaps even reorder to put the error case first:
>
> if (req_obj->err) {
> assert(!req_obj->req);
> rsp = qmp_error_response(req_obj->err);
> req_obj->err = NULL;
> monitor_qmp_respond(req_obj->mon, rsp, req_obj->id);
> qobject_unref(rsp);
> } else if (req_obj->req) {
> trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
> }
>
> Matter of taste.
Looks good to me. Thanks,
--
Peter Xu
