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