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 <sgarz...@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
