On Fri, Mar 03, 2017 at 08:38:45AM +0100, Markus Armbruster wrote:
> Niels de Vos <nde...@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 <arm...@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!
>
> >> &local_err);
> >> - if (!ptr) {
> >> - error_setg(&local_err, QERR_MISSING_PARAMETER,
> >> GLUSTER_OPT_TYPE);
> >> - error_append_hint(&local_err, GERR_INDEX_HINT, i);
> >> - goto out;
> >> -
> >> - }
> >> if (local_err) {
> >> error_append_hint(&local_err,
> >> "Parameter '%s' may be 'tcp' or 'unix'\n",
> >> @@ -626,8 +625,10 @@ static int
> >> qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
> >> curr->next->value = gsconf;
> >> curr = curr->next;
> >> }
> >> + gsconf = NULL;
> >>
> >> - qdict_del(backing_options, str);
> >> + QDECREF(backing_options);
> >> + backing_options = NULL;
> >> g_free(str);
> >> str = NULL;
> >> }
> >> @@ -636,11 +637,10 @@ static int
> >> qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
> >>
> >> out:
> >> error_propagate(errp, local_err);
> >> + qapi_free_GlusterServer(gsconf);
> >> qemu_opts_del(opts);
> >> - if (str) {
> >> - qdict_del(backing_options, str);
> >> - g_free(str);
> >> - }
> >> + g_free(str);
> >> + QDECREF(backing_options);
> >> errno = EINVAL;
> >> return -errno;
> >> }
> >> --
> >> 2.7.4
> >>
> >>
