Re: [Qemu-devel] [Qemu-block] [PATCH 12/15] gluster: Plug memory leaks in qemu_gluster_parse_json()

[Markus Armbruster <armbru@redhat.com>]

Niels de Vos <ndevos@redhat.com> writes:

> On Fri, Mar 03, 2017 at 08:38:45AM +0100, Markus Armbruster wrote:
>> Niels de Vos <ndevos@redhat.com> 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 <armbru@redhat.com>
>> >> ---
>> >>  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?