On Fri, Mar 03, 2017 at 09:35:16AM +0100, Markus Armbruster wrote: > Niels de Vos <nde...@redhat.com> writes: > > > 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! > > I'll change it. > > May I add your R-by then?
Yes, of course. Thanks, Niels