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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3305 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]