| Message ID | 1399404625-6093-4-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 05/06/2014 01:30 PM, Max Reitz wrote: > If the filename given to bdrv_open() is prefixed with "json:", parse the > rest as a JSON object and use the result as the options QDict. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > /* > * Opens a disk image (raw, qcow2, vmdk, ...) > * > @@ -1337,6 +1364,20 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, > options = qdict_new(); > } > > + if (filename && g_str_has_prefix(filename, "json:")) { > + QDict *json_options = parse_json_filename(filename, &local_err); > + if (local_err) { > + ret = -EINVAL; > + goto fail; > + } > + > + qdict_join(options, json_options, true); > + assert(qdict_size(json_options) == 0); Would it be better to pass false to qdict_join(), and then raise an error if the user specified conflicting options? For example (untested, just typing off the top of my head here), -drive file='json:{"driver":"qcow2","file.filename":"foo","backing.file.driver":"raw"}',backing.file.driver=qcow2 looks like it specifies conflicting backing.file.driver options. Passing true means that qdict_join silently overwrites the value in options to instead be the value in the json string; passing false means you could flag the user error. > + QDECREF(json_options); > + > + filename = NULL; > + } > + > bs->options = options; > options = qdict_clone_shallow(options); > >
On 06.05.2014 21:57, Eric Blake wrote: > On 05/06/2014 01:30 PM, Max Reitz wrote: >> If the filename given to bdrv_open() is prefixed with "json:", parse the >> rest as a JSON object and use the result as the options QDict. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 41 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 41 insertions(+) >> >> /* >> * Opens a disk image (raw, qcow2, vmdk, ...) >> * >> @@ -1337,6 +1364,20 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, >> options = qdict_new(); >> } >> >> + if (filename && g_str_has_prefix(filename, "json:")) { >> + QDict *json_options = parse_json_filename(filename, &local_err); >> + if (local_err) { >> + ret = -EINVAL; >> + goto fail; >> + } >> + >> + qdict_join(options, json_options, true); >> + assert(qdict_size(json_options) == 0); > Would it be better to pass false to qdict_join(), and then raise an > error if the user specified conflicting options? For example (untested, > just typing off the top of my head here), > > -drive > file='json:{"driver":"qcow2","file.filename":"foo","backing.file.driver":"raw"}',backing.file.driver=qcow2 > > looks like it specifies conflicting backing.file.driver options. > Passing true means that qdict_join silently overwrites the value in > options to instead be the value in the json string; passing false means > you could flag the user error. Yes, you're right; I'll change it. Max >> + QDECREF(json_options); >> + >> + filename = NULL; >> + } >> + >> bs->options = options; >> options = qdict_clone_shallow(options); >>
On 05/06/2014 02:00 PM, Max Reitz wrote: >> >> -drive >> file='json:{"driver":"qcow2","file.filename":"foo","backing.file.driver":"raw"}',backing.file.driver=qcow2 >> >> >> looks like it specifies conflicting backing.file.driver options. >> Passing true means that qdict_join silently overwrites the value in >> options to instead be the value in the json string; passing false means >> you could flag the user error. > > Yes, you're right; I'll change it. And of course, bonus points if you enhance the testsuite to provoke the error and prove we detect it :)
On 06.05.2014 22:28, Eric Blake wrote: > On 05/06/2014 02:00 PM, Max Reitz wrote: > >>> -drive >>> file='json:{"driver":"qcow2","file.filename":"foo","backing.file.driver":"raw"}',backing.file.driver=qcow2 >>> >>> >>> looks like it specifies conflicting backing.file.driver options. >>> Passing true means that qdict_join silently overwrites the value in >>> options to instead be the value in the json string; passing false means >>> you could flag the user error. >> Yes, you're right; I'll change it. > And of course, bonus points if you enhance the testsuite to provoke the > error and prove we detect it :) Already done. ;-) Max
Am 06.05.2014 um 21:57 hat Eric Blake geschrieben: > On 05/06/2014 01:30 PM, Max Reitz wrote: > > If the filename given to bdrv_open() is prefixed with "json:", parse the > > rest as a JSON object and use the result as the options QDict. > > > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > --- > > block.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > > > > /* > > * Opens a disk image (raw, qcow2, vmdk, ...) > > * > > @@ -1337,6 +1364,20 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, > > options = qdict_new(); > > } > > > > + if (filename && g_str_has_prefix(filename, "json:")) { > > + QDict *json_options = parse_json_filename(filename, &local_err); > > + if (local_err) { > > + ret = -EINVAL; > > + goto fail; > > + } > > + > > + qdict_join(options, json_options, true); > > + assert(qdict_size(json_options) == 0); > > Would it be better to pass false to qdict_join(), and then raise an > error if the user specified conflicting options? For example (untested, > just typing off the top of my head here), > > -drive > file='json:{"driver":"qcow2","file.filename":"foo","backing.file.driver":"raw"}',backing.file.driver=qcow2 > > looks like it specifies conflicting backing.file.driver options. > Passing true means that qdict_join silently overwrites the value in > options to instead be the value in the json string; passing false means > you could flag the user error. Isn't the more realistic case, that 'file' is actually the backing file string stored in an image, and the overwriting option comes from the command line? In this case, I think we want to allow overriding the option stored in the qcow2 file. Kevin
On 07.05.2014 10:39, Kevin Wolf wrote: > Am 06.05.2014 um 21:57 hat Eric Blake geschrieben: >> On 05/06/2014 01:30 PM, Max Reitz wrote: >>> If the filename given to bdrv_open() is prefixed with "json:", parse the >>> rest as a JSON object and use the result as the options QDict. >>> >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> block.c | 41 +++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 41 insertions(+) >>> >>> /* >>> * Opens a disk image (raw, qcow2, vmdk, ...) >>> * >>> @@ -1337,6 +1364,20 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, >>> options = qdict_new(); >>> } >>> >>> + if (filename && g_str_has_prefix(filename, "json:")) { >>> + QDict *json_options = parse_json_filename(filename, &local_err); >>> + if (local_err) { >>> + ret = -EINVAL; >>> + goto fail; >>> + } >>> + >>> + qdict_join(options, json_options, true); >>> + assert(qdict_size(json_options) == 0); >> Would it be better to pass false to qdict_join(), and then raise an >> error if the user specified conflicting options? For example (untested, >> just typing off the top of my head here), >> >> -drive >> file='json:{"driver":"qcow2","file.filename":"foo","backing.file.driver":"raw"}',backing.file.driver=qcow2 >> >> looks like it specifies conflicting backing.file.driver options. >> Passing true means that qdict_join silently overwrites the value in >> options to instead be the value in the json string; passing false means >> you could flag the user error. > Isn't the more realistic case, that 'file' is actually the backing file > string stored in an image, and the overwriting option comes from the > command line? In this case, I think we want to allow overriding the > option stored in the qcow2 file. Yes, you're probably right. I'll drop outputting an error message for conflicting entries from v2 in v3. Max
diff --git a/block.c b/block.c index b749d31..2555e34 100644 --- a/block.c +++ b/block.c @@ -1275,6 +1275,33 @@ out: g_free(tmp_filename); } +static QDict *parse_json_filename(const char *filename, Error **errp) +{ + QObject *options_obj; + QDict *options; + int ret; + + ret = strstart(filename, "json:", &filename); + assert(ret); + + options_obj = qobject_from_json(filename); + if (!options_obj) { + error_setg(errp, "Could not parse the JSON options"); + return NULL; + } + + if (qobject_type(options_obj) != QTYPE_QDICT) { + qobject_decref(options_obj); + error_setg(errp, "Invalid JSON object given"); + return NULL; + } + + options = qobject_to_qdict(options_obj); + qdict_flatten(options); + + return options; +} + /* * Opens a disk image (raw, qcow2, vmdk, ...) * @@ -1337,6 +1364,20 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, options = qdict_new(); } + if (filename && g_str_has_prefix(filename, "json:")) { + QDict *json_options = parse_json_filename(filename, &local_err); + if (local_err) { + ret = -EINVAL; + goto fail; + } + + qdict_join(options, json_options, true); + assert(qdict_size(json_options) == 0); + QDECREF(json_options); + + filename = NULL; + } + bs->options = options; options = qdict_clone_shallow(options);
If the filename given to bdrv_open() is prefixed with "json:", parse the rest as a JSON object and use the result as the options QDict. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)