From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54487) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZJzPT-00061c-Fv for qemu-devel@nongnu.org; Tue, 28 Jul 2015 03:34:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZJzPS-0002UO-07 for qemu-devel@nongnu.org; Tue, 28 Jul 2015 03:34:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52728) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZJzPR-0002UI-Pu for qemu-devel@nongnu.org; Tue, 28 Jul 2015 03:34:45 -0400 From: Markus Armbruster References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-30-git-send-email-armbru@redhat.com> <55B025B9.2060004@redhat.com> Date: Tue, 28 Jul 2015 09:34:42 +0200 In-Reply-To: <55B025B9.2060004@redhat.com> (Eric Blake's message of "Wed, 22 Jul 2015 17:22:33 -0600") Message-ID: <87zj2gohv1.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com Eric Blake writes: > On 07/01/2015 02:22 PM, Markus Armbruster wrote: >> is_c_ptr() looks whether the end of the C text for the type looks like >> a pointer. Works, but is fragile. >> >> We now have a better tool: use QAPISchemaType method c_null(). The >> initializers for non-pointers become prettier: 0, false or the >> enumeration constant with the value 0 instead of {0}. >> >> One place looks suspicious: we initialize pointers, but not >> non-pointers. Either the initialization is superfluous and should be >> deleted, or the non-pointers need it as well, or something subtle is >> going on and needs a comment. Since I lack the time to figure it out >> now, mark it FIXME. > > Commenting on just this part for now (out of time to review this patch > proper for today): > >> @@ -214,7 +208,8 @@ def gen_marshal_input(name, args, ret_type, middle_mode): >> header=hdr) >> >> if ret_type: >> - if is_c_ptr(ret_type.c_type()): >> + # FIXME fishy: only pointers are initialized >> + if ret_type.c_null() == 'NULL': >> retval = " %s retval = NULL;" % ret_type.c_type() >> else: >> retval = " %s retval;" % ret_type.c_type() > > Here's an example function impacted by this: > > static void qmp_marshal_input_guest_file_open(QDict *args, QObject > **ret, Error > **errp) > { > Error *local_err = NULL; > int64_t retval; > QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args)); > QapiDeallocVisitor *md; > Visitor *v; > char *path = NULL; > bool has_mode = false; > char *mode = NULL; > ... > retval = qmp_guest_file_open(path, has_mode, mode, &local_err); > if (local_err) { > goto out; > } > > qmp_marshal_output_guest_file_open(retval, ret, &local_err); > > But compare that to any other function that returns a pointer, and > you'll see the same pattern (the only use of retval is in the final call > to qmp_marshal_output..., right after assigning retval); that is, Correct. Inspection of qapi-commands.py shows retval is only ever read in the call of qmp_marshal_output_FOO(). > initializing to NULL is dead code, and you could get away with just > always declaring it instead of worrying about c_null() in this code. > > Or maybe we have a leak - if the 'if (local_err)' can ever trigger even > when a function returned non-NULL, then our out: label is missing a > free(retval), but only when retval is a pointer. Or maybe we change the > code to assert that retval is NULL if local_err is set after calling the > user's function, to prove we don't have a leak. Let me rephrase to make sure I understand. Ignore the (not rets) case, because retval doesn't exist then. qmp_marshal_output_FOO() visits retval twice. First, with a QMP output visitor to do the actual marshalling. Second, with a QAPI dealloc visitor to destroy it. If we execute the assignment to retval, we must go on to call qmp_marshal_output_FOO(), or else we have a leak. If we can reach qmp_marshal_output_FOO() without executing the assignment, we must initialize retval. If we can't, any initialization is unused. gen_call() generates code of the form retval = qmp_FOO(... args ..., &local_err); if (local_err) { goto out; } qmp_marshal_output_FOO(retval, ret, &local_err); Its caller then generates out: error_propagate(errp, local_err); and so forth. Observe: 1. The assignment dominates the only use. Therefore, the initialization is unused. Let's drop it in a separate cleanup patch. 2. We can leak retval only when qmp_FOO() returns non-null and local_err is non-null. This must not happen, because: a. local_err must be null before the call, and b. the call must not return non-null when it sets local_err. We could right after out: assert(!local_err || !retval). Not sure it's worthwhile. TL;DR: I concur with your analysis.