Re: [Qemu-devel] [PATCH] gluster: fix .bdrv_reopen_prepare when backing file is a JSON object

[Stefano Garzarella]

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 <address@hidden>
> >>>>> ---
> >>>>>  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