[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 19/37] qmp: Fix reference-counting of qnull o
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v9 19/37] qmp: Fix reference-counting of qnull on empty output visit |
Date: |
Thu, 21 Jan 2016 14:19:35 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Markus Armbruster <address@hidden> writes:
> Eric Blake <address@hidden> writes:
>
>> Commit 6c2f9a15 ensured that we would not return NULL when the
>> caller used an output visitor but had nothing to visit. But
>> in doing so, it added a FIXME about a reference count leak
>> that could abort qemu in the (unlikely) case of SIZE_MAX such
>> visits (more plausible on 32-bit). (Although that commit
>> suggested we might fix it in time for 2.5, we ran out of time;
>> fortunately, it is unlikely enough to bite that it was not
>> worth worrying about during the 2.5 release.)
>>
>> This fixes things by documenting the internal contracts, and
>> explaining why the internal function can return NULL and only
>> the public facing interface needs to worry about qnull(),
>> thus avoiding over-referencing the qnull_ global object.
>>
>> It does not, however, fix the stupidity of the stack mixing
>> up two separate pieces of information; add a FIXME to explain
>> that issue, which will be fixed shortly in a future patch.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> Cc: address@hidden
>> Reviewed-by: Marc-André Lureau <address@hidden>
>>
>> ---
>> v9: enhance commit message
>> v8: rebase to earlier changes
>> v7: cc qemu-stable, tweak some asserts, drop stale comment, add more
>> comments
>> v6: no change
>> ---
>> qapi/qmp-output-visitor.c | 39 ++++++++++++++++++++++++++++++++-------
>> tests/test-qmp-output-visitor.c | 2 ++
>> 2 files changed, 34 insertions(+), 7 deletions(-)
>>
>> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
>> index d367148..316f4e4 100644
>> --- a/qapi/qmp-output-visitor.c
>> +++ b/qapi/qmp-output-visitor.c
[...]
>> @@ -41,10 +50,12 @@ static QmpOutputVisitor *to_qov(Visitor *v)
>> return container_of(v, QmpOutputVisitor, visitor);
>> }
>>
>> +/* Push @value onto the stack of current QObjects being built */
>> static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
>> {
>> QStackEntry *e = g_malloc0(sizeof(*e));
>>
>> + assert(value);
>> e->value = value;
>> if (qobject_type(e->value) == QTYPE_QLIST) {
>> e->is_list_head = true;
>> @@ -52,44 +63,51 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov,
>> QObject *value)
>> QTAILQ_INSERT_HEAD(&qov->stack, e, node);
>> }
>>
>> +/* Grab and remove the most recent QObject from the stack */
Let's stick to the stack terminology you used for qmp_output_push_obj():
/* Pop a value off the stack of QObjects being built, and return it */
>> static QObject *qmp_output_pop(QmpOutputVisitor *qov)
>> {
>> QStackEntry *e = QTAILQ_FIRST(&qov->stack);
>> QObject *value;
>> +
>> + assert(e);
>> QTAILQ_REMOVE(&qov->stack, e, node);
>> value = e->value;
>> g_free(e);
>> return value;
>
> @value cannot be null here, because we never push nulls. Worth an
> assertion, just to help readers?
>
>> }
[...]
>> +/* Grab the most recent QObject from the stack, which must exist */
Stack terminology:
/*
* Peek at the top of the stack of QObject being built.
* The stack must not be empty.
*/
>> static QObject *qmp_output_last(QmpOutputVisitor *qov)
>> {
>> QStackEntry *e = QTAILQ_FIRST(&qov->stack);
>> +
>> + assert(e && e->value);
>> return e->value;
>> }
[...]
[Qemu-devel] [PATCH v9 32/37] qapi: Rework deallocation of partial struct, Eric Blake, 2016/01/19