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

[Stefano Garzarella <sgarzare@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