Re: [Qemu-devel] [PATCH v7 20/26] qapi: Make output visitor return qnull() instead of NULL

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> On Mon, Sep 14, 2015 at 07:57:51PM +0200, Markus Armbruster wrote:
>> Before commit 1d10b44, it crashed.  Since then, it returns NULL, with
>> a FIXME comment.  The FIXME is valid: code that assumes QObject *
>> can't be null exists.  I'm not aware of a way to feed this problematic
>> return value to code that actually chokes on null in the current code,
>> but the next few commits will create one, failing "make check".
>> 
>> Commit 481b002 solved a very similar problem by introducing a special
>> null QObject.  Using this special null QObject is clearly the right
>> way to resolve this FIXME, so do that, and update the test
>> accordingly.
>> 
>> However, the patch isn't quite right: it messes up the reference
>> counting.  After about SIZE_MAX visits, the reference counter
>> overflows, failing the assertion in qnull_destroy_obj().  Because
>> that's many orders of magnitude more visits of nulls than we expect,
>> we take this patch despite its flaws, to get the QMP introspection
>> stuff in without further delay.
>> 
>> Naturally, we'll have to fix it for real before the release.
>
> Do we actually ever get near to SIZE_MAX visits ? If not, then
> it would not seem critical to fix before release, as this is
> just the generator code

SIZE_MAX visits seem unlikely even when SIZE_MAX is only 2^32-1.  It
would be fatal, though: QEMU would crash.

I'll reword to "we'll want to fix it".

>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  qapi/qmp-output-visitor.c       | 8 ++++++--
>>  tests/test-qmp-output-visitor.c | 3 ++-
>>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> Reviewed-by: Daniel P. Berrange <address@hidden>

Thanks!