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). Max
signature.asc
Description: OpenPGP digital signature
