On 07/11/13 21:14, Eric Blake wrote:
> On 07/11/2013 12:50 PM, Luiz Capitulino wrote:
>> I'm sending this as an RFC because this is untested, and also because
>> I'm wondering if I'm seeing things after a long patch review session.
>
> I can't say that I tested it either, but...
>
>>
>> The problem is: in qmp-marshal.c, the dealloc visitor calls use the
>> same errp pointer of the input visitor calls. This means that if
>> any of the input visitor calls fails, then the dealloc visitor will
>> return early, beforing freeing the object's memory.
It's a good idea to fix this.
>
> s/beforing/before/
>
>>
>> Here's an example, consider this code:
>>
>> int qmp_marshal_input_block_passwd(Monitor *mon, const QDict *qdict, QObject
>> **ret)
>> {
>> [...]
>>
>> char * device = NULL;
>> char * password = NULL;
>>
>> mi = qmp_input_visitor_new_strict(QOBJECT(args));
>> v = qmp_input_get_visitor(mi);
>> visit_type_str(v, &device, "device", errp);
>> visit_type_str(v, &password, "password", errp);
>> qmp_input_visitor_cleanup(mi);
>>
>> if (error_is_set(errp)) {
>> goto out;
>> }
>> qmp_block_passwd(device, password, errp);
>>
>> out:
>> md = qapi_dealloc_visitor_new();
>> v = qapi_dealloc_get_visitor(md);
>> visit_type_str(v, &device, "device", errp);
>
> I definitely agree that the current generated code passes in a non-null
> errp, and that visit_type_str is a no-op when started in an existing error.
>
>> visit_type_str(v, &password, "password", errp);
>> qapi_dealloc_visitor_cleanup(md);
>>
>> [...]
>>
>> return 0;
>> }
>>
>> Consider errp != NULL when the out label is reached, we're going
>> to leak device and password.
>>
>> This patch fixes this by always passing errp=NULL for dealloc
>> visitors, meaning that we always try to free them regardless of
>> any previous failure.
I agree with that.
> The above example would then be:
>>
>> out:
>> md = qapi_dealloc_visitor_new();
>> v = qapi_dealloc_get_visitor(md);
>> visit_type_str(v, &device, "device", NULL);
>> visit_type_str(v, &password, "password", NULL);
>> qapi_dealloc_visitor_cleanup(md);
>
> Is that safe even if the failure was after device was parsed, meaning
> the initial visitor to password was a no-op and there is nothing to
> deallocate for password? I _think_ this is a correct fix (it means that
> errors encountered only while doing a dealloc pass are lost, but what
> errors are you going to encounter in that direction?); but I'd feel more
> comfortable is someone else more familiar with visitors chimes in.
Two points:
(a) passing NULL "errp"s to the dealloc traversal also prevents the
dealloc traversal to set an error mid-way, and to abort the traversal.
However this is perfectly fine, the dealloc traversal (in parts or in
entirity) should never fail.
(Cf. you can't throw an exception in a C++ destructor -- the destructor
could be running as part of exception propagation already.)
(b) The generated traversal code, independently of the visitor object,
can (should!) deal with *arbitrarily* incomplete trees since
commit d195325b05199038b5907fa791729425b9720d21
Author: Paolo Bonzini <pbonz...@redhat.com>
Date: Tue Jul 17 16:17:04 2012 +0200
qapi: fix error propagation
>
>>
>> Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
>> ---
>> scripts/qapi-commands.py | 17 ++++++++++-------
>> 1 file changed, 10 insertions(+), 7 deletions(-)
>>
>
>> +visit_start_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
>> if (has_%(c_name)s) {
>> ''',
>> - c_name=c_var(argname), name=argname)
>> + c_name=c_var(argname), name=argname,errp=errparg)
>
> Any reason you don't use space after ',' (several instances)?
>
With the spaces fixed:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
