From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43278) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cjigR-0008Jo-O5 for qemu-devel@nongnu.org; Fri, 03 Mar 2017 03:35:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cjigQ-0005d3-Me for qemu-devel@nongnu.org; Fri, 03 Mar 2017 03:35:27 -0500 From: Markus Armbruster References: <1488491046-2549-1-git-send-email-armbru@redhat.com> <1488491046-2549-13-git-send-email-armbru@redhat.com> <20170303071125.GS16140@ndevos-x240.usersys.redhat.com> <87pohy4xoa.fsf@dusky.pond.sub.org> <20170303081743.GU16140@ndevos-x240.usersys.redhat.com> Date: Fri, 03 Mar 2017 09:35:16 +0100 In-Reply-To: <20170303081743.GU16140@ndevos-x240.usersys.redhat.com> (Niels de Vos's message of "Fri, 3 Mar 2017 00:17:43 -0800") Message-ID: <87efye3ghn.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 12/15] gluster: Plug memory leaks in qemu_gluster_parse_json() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Niels de Vos Cc: kwolf@redhat.com, mitake.hitoshi@lab.ntt.co.jp, qemu-devel@nongnu.org, qemu-block@nongnu.org, namei.unix@gmail.com Niels de Vos writes: > On Fri, Mar 03, 2017 at 08:38:45AM +0100, Markus Armbruster wrote: >> Niels de Vos writes: >> >> > On Thu, Mar 02, 2017 at 10:44:03PM +0100, Markus Armbruster wrote: >> >> To reproduce, run >> >> >> >> $ valgrind qemu-system-x86_64 --nodefaults -S --drive driver=gluster,volume=testvol,path=/a/b/c,server.0.type=xxx >> >> >> >> Signed-off-by: Markus Armbruster >> >> --- >> >> block/gluster.c | 28 ++++++++++++++-------------- >> >> 1 file changed, 14 insertions(+), 14 deletions(-) >> >> >> >> diff --git a/block/gluster.c b/block/gluster.c >> >> index 6fbcf9e..35a7abb 100644 >> >> --- a/block/gluster.c >> >> +++ b/block/gluster.c >> >> @@ -480,7 +480,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, >> >> QDict *options, Error **errp) >> >> { >> >> QemuOpts *opts; >> >> - GlusterServer *gsconf; >> >> + GlusterServer *gsconf = NULL; >> >> GlusterServerList *curr = NULL; >> >> QDict *backing_options = NULL; >> >> Error *local_err = NULL; >> >> @@ -529,17 +529,16 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, >> >> } >> >> >> >> ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE); >> >> + if (!ptr) { >> >> + error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE); >> >> + error_append_hint(&local_err, GERR_INDEX_HINT, i); >> >> + goto out; >> >> + >> >> + } >> >> gsconf = g_new0(GlusterServer, 1); >> >> gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr, >> >> - GLUSTER_TRANSPORT__MAX, >> >> - GLUSTER_TRANSPORT__MAX, >> >> + GLUSTER_TRANSPORT__MAX, 0, >> > >> > What is the reason to set the default to 0 and not the more readable >> > GLUSTER_TRANSPORT_UNIX? >> >> I chose 0 because the actual value must not matter: we don't want a >> default here. >> >> qapi_enum_parse() returns this argument when ptr isn't found. If ptr is >> non-null, it additionally sets an error. Since ptr can't be null here, >> the argument is only returned together with an error. But then we won't >> use *gsconf. > > Ah, right, I that see now. > >> Do you think -1 instead of 0 would be clearer? > > Yes, it would be to me. -1 is clearly not part of the GLUSTER_TRANSPORT_* > enum, so it suggests it is a different case. > > Thanks! I'll change it. May I add your R-by then?