| Message ID | 1488491046-2549-13-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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? > &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 > >
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. Do you think -1 instead of 0 would be clearer? >> &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 >> >>
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! > > >> &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 > >> > >>
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?
On Fri, Mar 03, 2017 at 09:35:16AM +0100, Markus Armbruster wrote: > 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? Yes, of course. Thanks, Niels
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, &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; }
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(-)