Re: [Qemu-devel] [PATCH 14/32] qmp: Always free QMPRequest with qmp_request_free()
Eric Blake writes: > On 07/02/2018 11:22 AM, Markus Armbruster wrote: >> monitor_qmp_dispatch_one() frees a QMPRequest manually, because it >> needs to keep a reference to ->id. Premature optimization. Take an >> additional reference so we can use qmp_request_free(). >> >> Signed-off-by: Markus Armbruster >> --- >> monitor.c | 6 ++ >> 1 file changed, 2 insertions(+), 4 deletions(-) >> > > Reviewed-by: Eric Blake > >> diff --git a/monitor.c b/monitor.c >> index 10991757f6..94f5660c3c 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -4181,8 +4181,6 @@ static void monitor_qmp_dispatch_one(QMPRequest >> *req_obj) >> id = req_obj->id; >> need_resume = req_obj->need_resume; >> -g_free(req_obj); >> - >> old_mon = cur_mon; >> cur_mon = mon; >> @@ -4191,14 +4189,14 @@ static void >> monitor_qmp_dispatch_one(QMPRequest *req_obj) >> cur_mon = old_mon; >> /* Respond if necessary */ >> -monitor_qmp_respond(mon, rsp, NULL, id); >> +monitor_qmp_respond(mon, rsp, NULL, qobject_ref(id)); > > This requires qobject_ref(NULL) to work (I thought at one point we > were considering requiring it to be a non-NULL argument, but right now > it looks like we are safe). [PATCH v6 5/5] qobject: modify qobject_ref() to assert on NULL Message-Id: <20180419150145.24795-6-marcandre.lur...@redhat.com> I didn't merge it back then because I lacked the time to convince myself all users of qobject_ref() the patch doesn't guard cannot pass null, and also because I wasn't sure requiring qobject_ref()'s argument to be non-null improves legibility of the code. Right now we got bigger fish to fry.
Re: [Qemu-devel] [PATCH 14/32] qmp: Always free QMPRequest with qmp_request_free()
On 07/02/2018 11:22 AM, Markus Armbruster wrote: monitor_qmp_dispatch_one() frees a QMPRequest manually, because it needs to keep a reference to ->id. Premature optimization. Take an additional reference so we can use qmp_request_free(). Signed-off-by: Markus Armbruster --- monitor.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) Reviewed-by: Eric Blake diff --git a/monitor.c b/monitor.c index 10991757f6..94f5660c3c 100644 --- a/monitor.c +++ b/monitor.c @@ -4181,8 +4181,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj) id = req_obj->id; need_resume = req_obj->need_resume; -g_free(req_obj); - old_mon = cur_mon; cur_mon = mon; @@ -4191,14 +4189,14 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj) cur_mon = old_mon; /* Respond if necessary */ -monitor_qmp_respond(mon, rsp, NULL, id); +monitor_qmp_respond(mon, rsp, NULL, qobject_ref(id)); This requires qobject_ref(NULL) to work (I thought at one point we were considering requiring it to be a non-NULL argument, but right now it looks like we are safe). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
