| Message ID | 20190712104617.94707-1-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gluster: fix .bdrv_reopen_prepare when backing file is a JSON object | expand |
On 12.07.19 12:46, Stefano Garzarella wrote: > When the backing_file is specified as a JSON object, the > qemu_gluster_reopen_prepare() fails with this message: > invalid URI json:{"server.0.host": ...} > > In this case, we should call qemu_gluster_init() using the QDict > 'state->options' that contains the parameters already parsed. > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1542445 > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > block/gluster.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/block/gluster.c b/block/gluster.c > index 62f8ff2147..26971db1ea 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -931,7 +931,16 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, > gconf->has_debug = true; > gconf->logfile = g_strdup(s->logfile); > gconf->has_logfile = true; > - reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp); > + /* > + * If 'bs->filename' starts with "json:", then 'state->options' will > + * contain the parameters already parsed. > + */ > + if (state->bs->filename && !strstart(state->bs->filename, "json:", NULL)) { > + reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, > + errp); > + } else { > + reop_s->glfs = qemu_gluster_init(gconf, NULL, state->options, errp); > + } Hmmm, aren’t they always in state->options? Max
Patchew URL: https://patchew.org/QEMU/20190712104617.94707-1-sgarzare@redhat.com/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
CC audio/noaudio.o
CC audio/wavaudio.o
CC audio/mixeng.o
/tmp/qemu-test/src/block/gluster.c:938:20: error: address of array 'state->bs->filename' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
if (state->bs->filename && !strstart(state->bs->filename, "json:", NULL)) {
~~~~~~~~~~~^~~~~~~~ ~~
1 error generated.
The full log is available at
http://patchew.org/logs/20190712104617.94707-1-sgarzare@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Fri, Jul 12, 2019 at 08:35:12PM +0200, Max Reitz wrote: > On 12.07.19 12:46, Stefano Garzarella wrote: > > When the backing_file is specified as a JSON object, the > > qemu_gluster_reopen_prepare() fails with this message: > > invalid URI json:{"server.0.host": ...} > > > > In this case, we should call qemu_gluster_init() using the QDict > > 'state->options' that contains the parameters already parsed. > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1542445 > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > --- > > block/gluster.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/block/gluster.c b/block/gluster.c > > index 62f8ff2147..26971db1ea 100644 > > --- a/block/gluster.c > > +++ b/block/gluster.c > > @@ -931,7 +931,16 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, > > gconf->has_debug = true; > > gconf->logfile = g_strdup(s->logfile); > > gconf->has_logfile = true; > > - reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp); > > + /* > > + * If 'bs->filename' starts with "json:", then 'state->options' will > > + * contain the parameters already parsed. > > + */ > > + if (state->bs->filename && !strstart(state->bs->filename, "json:", NULL)) { > > + reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, > > + errp); > > + } else { > > + reop_s->glfs = qemu_gluster_init(gconf, NULL, state->options, errp); > > + } > > Hmmm, aren’t they always in state->options? Yes, you are rigth, but the qemu_gluster_parse() doesn't search for the 'filename' in the QDict *options. Maybe I can simply modify it in this way in order to hanlde this case, calling qemu_gluster_init() only with 'state->options'. diff --git a/block/gluster.c b/block/gluster.c index 26971db1ea..91d674cd2b 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -695,6 +695,11 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf, QDict *options, Error **errp) { int ret; + + if (!filename) { + filename = qdict_get_try_str(options, GLUSTER_OPT_FILENAME); + } + if (filename) { ret = qemu_gluster_parse_uri(gconf, filename); if (ret < 0) { Do you think it is better/cleaner? Thanks, Stefano
On 15.07.19 10:16, Stefano Garzarella wrote: > On Fri, Jul 12, 2019 at 08:35:12PM +0200, Max Reitz wrote: >> On 12.07.19 12:46, Stefano Garzarella wrote: >>> When the backing_file is specified as a JSON object, the >>> qemu_gluster_reopen_prepare() fails with this message: >>> invalid URI json:{"server.0.host": ...} >>> >>> In this case, we should call qemu_gluster_init() using the QDict >>> 'state->options' that contains the parameters already parsed. >>> >>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1542445 >>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>> --- >>> block/gluster.c | 11 ++++++++++- >>> 1 file changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/block/gluster.c b/block/gluster.c >>> index 62f8ff2147..26971db1ea 100644 >>> --- a/block/gluster.c >>> +++ b/block/gluster.c >>> @@ -931,7 +931,16 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, >>> gconf->has_debug = true; >>> gconf->logfile = g_strdup(s->logfile); >>> gconf->has_logfile = true; >>> - reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp); >>> + /* >>> + * If 'bs->filename' starts with "json:", then 'state->options' will >>> + * contain the parameters already parsed. >>> + */ >>> + if (state->bs->filename && !strstart(state->bs->filename, "json:", NULL)) { >>> + reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, >>> + errp); >>> + } else { >>> + reop_s->glfs = qemu_gluster_init(gconf, NULL, state->options, errp); >>> + } >> >> Hmmm, aren’t they always in state->options? > > Yes, you are rigth, but the qemu_gluster_parse() doesn't search for the > 'filename' in the QDict *options. > > Maybe I can simply modify it in this way in order to hanlde this case, > calling qemu_gluster_init() only with 'state->options'. > > diff --git a/block/gluster.c b/block/gluster.c > index 26971db1ea..91d674cd2b 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -695,6 +695,11 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf, > QDict *options, Error **errp) > { > int ret; > + > + if (!filename) { > + filename = qdict_get_try_str(options, GLUSTER_OPT_FILENAME); > + } > + > if (filename) { > ret = qemu_gluster_parse_uri(gconf, filename); > if (ret < 0) { > > > Do you think it is better/cleaner? No, because the rest of the function completely ignores @options if @filename is set. Hm. I can’t think of anything better than your original solution, then. Except the “state->bs->filename” should probably be a “state->bs->filename[0]” (as Patchew has pointed out). Max
On Mon, Jul 15, 2019 at 12:53:57PM +0200, Max Reitz wrote: > On 15.07.19 10:16, Stefano Garzarella wrote: > > On Fri, Jul 12, 2019 at 08:35:12PM +0200, Max Reitz wrote: > >> On 12.07.19 12:46, Stefano Garzarella wrote: > >>> When the backing_file is specified as a JSON object, the > >>> qemu_gluster_reopen_prepare() fails with this message: > >>> invalid URI json:{"server.0.host": ...} > >>> > >>> In this case, we should call qemu_gluster_init() using the QDict > >>> 'state->options' that contains the parameters already parsed. > >>> > >>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1542445 > >>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > >>> --- > >>> block/gluster.c | 11 ++++++++++- > >>> 1 file changed, 10 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/block/gluster.c b/block/gluster.c > >>> index 62f8ff2147..26971db1ea 100644 > >>> --- a/block/gluster.c > >>> +++ b/block/gluster.c > >>> @@ -931,7 +931,16 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, > >>> gconf->has_debug = true; > >>> gconf->logfile = g_strdup(s->logfile); > >>> gconf->has_logfile = true; > >>> - reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp); > >>> + /* > >>> + * If 'bs->filename' starts with "json:", then 'state->options' will > >>> + * contain the parameters already parsed. > >>> + */ > >>> + if (state->bs->filename && !strstart(state->bs->filename, "json:", NULL)) { > >>> + reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, > >>> + errp); > >>> + } else { > >>> + reop_s->glfs = qemu_gluster_init(gconf, NULL, state->options, errp); > >>> + } > >> > >> Hmmm, aren’t they always in state->options? > > > > Yes, you are rigth, but the qemu_gluster_parse() doesn't search for the > > 'filename' in the QDict *options. > > > > Maybe I can simply modify it in this way in order to hanlde this case, > > calling qemu_gluster_init() only with 'state->options'. > > > > diff --git a/block/gluster.c b/block/gluster.c > > index 26971db1ea..91d674cd2b 100644 > > --- a/block/gluster.c > > +++ b/block/gluster.c > > @@ -695,6 +695,11 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf, > > QDict *options, Error **errp) > > { > > int ret; > > + > > + if (!filename) { > > + filename = qdict_get_try_str(options, GLUSTER_OPT_FILENAME); > > + } > > + > > if (filename) { > > ret = qemu_gluster_parse_uri(gconf, filename); > > if (ret < 0) { > > > > > > Do you think it is better/cleaner? > > No, because the rest of the function completely ignores @options if > @filename is set. > > Hm. I can’t think of anything better than your original solution, then. > Except the “state->bs->filename” should probably be a > “state->bs->filename[0]” (as Patchew has pointed out). Sure, I'll fix it in the v2. Thanks, Stefano
On 15.07.19 14:50, Stefano Garzarella wrote: > On Mon, Jul 15, 2019 at 12:53:57PM +0200, Max Reitz wrote: >> On 15.07.19 10:16, Stefano Garzarella wrote: >>> On Fri, Jul 12, 2019 at 08:35:12PM +0200, Max Reitz wrote: >>>> On 12.07.19 12:46, Stefano Garzarella wrote: >>>>> When the backing_file is specified as a JSON object, the >>>>> qemu_gluster_reopen_prepare() fails with this message: >>>>> invalid URI json:{"server.0.host": ...} >>>>> >>>>> In this case, we should call qemu_gluster_init() using the QDict >>>>> 'state->options' that contains the parameters already parsed. >>>>> >>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1542445 >>>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>>>> --- >>>>> block/gluster.c | 11 ++++++++++- >>>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/block/gluster.c b/block/gluster.c >>>>> index 62f8ff2147..26971db1ea 100644 >>>>> --- a/block/gluster.c >>>>> +++ b/block/gluster.c >>>>> @@ -931,7 +931,16 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, >>>>> gconf->has_debug = true; >>>>> gconf->logfile = g_strdup(s->logfile); >>>>> gconf->has_logfile = true; >>>>> - reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp); >>>>> + /* >>>>> + * If 'bs->filename' starts with "json:", then 'state->options' will >>>>> + * contain the parameters already parsed. >>>>> + */ >>>>> + if (state->bs->filename && !strstart(state->bs->filename, "json:", NULL)) { >>>>> + reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, >>>>> + errp); >>>>> + } else { >>>>> + reop_s->glfs = qemu_gluster_init(gconf, NULL, state->options, errp); >>>>> + } >>>> >>>> Hmmm, aren’t they always in state->options? >>> >>> Yes, you are rigth, but the qemu_gluster_parse() doesn't search for the >>> 'filename' in the QDict *options. >>> >>> Maybe I can simply modify it in this way in order to hanlde this case, >>> calling qemu_gluster_init() only with 'state->options'. >>> >>> diff --git a/block/gluster.c b/block/gluster.c >>> index 26971db1ea..91d674cd2b 100644 >>> --- a/block/gluster.c >>> +++ b/block/gluster.c >>> @@ -695,6 +695,11 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf, >>> QDict *options, Error **errp) >>> { >>> int ret; >>> + >>> + if (!filename) { >>> + filename = qdict_get_try_str(options, GLUSTER_OPT_FILENAME); >>> + } >>> + >>> if (filename) { >>> ret = qemu_gluster_parse_uri(gconf, filename); >>> if (ret < 0) { >>> >>> >>> Do you think it is better/cleaner? >> >> No, because the rest of the function completely ignores @options if >> @filename is set. >> >> Hm. I can’t think of anything better than your original solution, then. >> Except the “state->bs->filename” should probably be a >> “state->bs->filename[0]” (as Patchew has pointed out). > > Sure, I'll fix it in the v2. Oh, wait. You can actually just use state->bs->exact_filename. That will never be a json:{} filename, so you don’t have to do the prefix check (just see whether it is empty). Max
On Mon, Jul 15, 2019 at 03:00:29PM +0200, Max Reitz wrote: > On 15.07.19 14:50, Stefano Garzarella wrote: > > On Mon, Jul 15, 2019 at 12:53:57PM +0200, Max Reitz wrote: > >> On 15.07.19 10:16, Stefano Garzarella wrote: > >>> On Fri, Jul 12, 2019 at 08:35:12PM +0200, Max Reitz wrote: > >>>> On 12.07.19 12:46, Stefano Garzarella wrote: > >>>>> When the backing_file is specified as a JSON object, the > >>>>> qemu_gluster_reopen_prepare() fails with this message: > >>>>> invalid URI json:{"server.0.host": ...} > >>>>> > >>>>> In this case, we should call qemu_gluster_init() using the QDict > >>>>> 'state->options' that contains the parameters already parsed. > >>>>> > >>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1542445 > >>>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > >>>>> --- > >>>>> block/gluster.c | 11 ++++++++++- > >>>>> 1 file changed, 10 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/block/gluster.c b/block/gluster.c > >>>>> index 62f8ff2147..26971db1ea 100644 > >>>>> --- a/block/gluster.c > >>>>> +++ b/block/gluster.c > >>>>> @@ -931,7 +931,16 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, > >>>>> gconf->has_debug = true; > >>>>> gconf->logfile = g_strdup(s->logfile); > >>>>> gconf->has_logfile = true; > >>>>> - reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp); > >>>>> + /* > >>>>> + * If 'bs->filename' starts with "json:", then 'state->options' will > >>>>> + * contain the parameters already parsed. > >>>>> + */ > >>>>> + if (state->bs->filename && !strstart(state->bs->filename, "json:", NULL)) { > >>>>> + reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, > >>>>> + errp); > >>>>> + } else { > >>>>> + reop_s->glfs = qemu_gluster_init(gconf, NULL, state->options, errp); > >>>>> + } > >>>> > >>>> Hmmm, aren’t they always in state->options? > >>> > >>> Yes, you are rigth, but the qemu_gluster_parse() doesn't search for the > >>> 'filename' in the QDict *options. > >>> > >>> Maybe I can simply modify it in this way in order to hanlde this case, > >>> calling qemu_gluster_init() only with 'state->options'. > >>> > >>> diff --git a/block/gluster.c b/block/gluster.c > >>> index 26971db1ea..91d674cd2b 100644 > >>> --- a/block/gluster.c > >>> +++ b/block/gluster.c > >>> @@ -695,6 +695,11 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf, > >>> QDict *options, Error **errp) > >>> { > >>> int ret; > >>> + > >>> + if (!filename) { > >>> + filename = qdict_get_try_str(options, GLUSTER_OPT_FILENAME); > >>> + } > >>> + > >>> if (filename) { > >>> ret = qemu_gluster_parse_uri(gconf, filename); > >>> if (ret < 0) { > >>> > >>> > >>> Do you think it is better/cleaner? > >> > >> No, because the rest of the function completely ignores @options if > >> @filename is set. > >> > >> Hm. I can’t think of anything better than your original solution, then. > >> Except the “state->bs->filename” should probably be a > >> “state->bs->filename[0]” (as Patchew has pointed out). > > > > Sure, I'll fix it in the v2. > > Oh, wait. You can actually just use state->bs->exact_filename. That > will never be a json:{} filename, so you don’t have to do the prefix > check (just see whether it is empty). Yeah, thanks for the suggestion! Stefano
diff --git a/block/gluster.c b/block/gluster.c index 62f8ff2147..26971db1ea 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -931,7 +931,16 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, gconf->has_debug = true; gconf->logfile = g_strdup(s->logfile); gconf->has_logfile = true; - reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp); + /* + * If 'bs->filename' starts with "json:", then 'state->options' will + * contain the parameters already parsed. + */ + if (state->bs->filename && !strstart(state->bs->filename, "json:", NULL)) { + reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, + errp); + } else { + reop_s->glfs = qemu_gluster_init(gconf, NULL, state->options, errp); + } if (reop_s->glfs == NULL) { ret = -errno; goto exit;
When the backing_file is specified as a JSON object, the qemu_gluster_reopen_prepare() fails with this message: invalid URI json:{"server.0.host": ...} In this case, we should call qemu_gluster_init() using the QDict 'state->options' that contains the parameters already parsed. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1542445 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- block/gluster.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)