On 03/30/2017 08:15 AM, Markus Armbruster wrote: > However, A few places access the flat QDict directly: > > * Most of them access members that are always QString. Correct. > > * bdrv_open_inherit() accesses a boolean, carefully. Correct. > > * nfs_config() uses a QObject input visitor. Correct only because the > visited type contains nothing but QStrings. > > * nbd_config() and ssh_config() use a QObject input visitor, and the > visited types contain non-QStrings: InetSocketAddress members > @numeric, @to, @ipv4, @ipv6. -drive works as long as you don't try > to use them (they're all optional). @to is ignored anyway. > > Reproducer: > -drive driver=ssh,server.host=h,server.port=22,server.ipv4,path=p > -drive > driver=nbd,server.type=inet,server.data.host=h,server.data.port=22,server.data.ipv4 > both fail with "Invalid parameter type for 'data.ipv4', expected: boolean" > > Add suitable comments to all these places. Mark the buggy ones FIXME. > > "Fortunately", -drive's driver-specific options are entirely > undocumented. > > Signed-off-by: Markus Armbruster <arm...@redhat.com> > ---
> +++ b/block.c
> @@ -1157,6 +1157,12 @@ static int bdrv_open_common(BlockDriverState *bs,
> BlockBackend *file,
> if (file != NULL) {
> filename = blk_bs(file)->filename;
> } else {
> + /*
> + * Caution: direct use of non-string @options members is
> + * problematic. When they come from -blockdev or blockdev_add,
> + * members are typed according to the QAPI schema, but when
> + * they come from -drive, they're all QString.
> + */
> filename = qdict_get_try_str(options, "filename");
Did we get the latest round of comment tweaking in here? I was
expecting to see something along the lines of:
"Caution: accessing 'filename' from @options here is safe, but direct
use of any non-string @options members would be problematic. When they
come..."
> }
>
> @@ -1324,6 +1330,12 @@ static int bdrv_fill_options(QDict **options, const
> char *filename,
> BlockDriver *drv = NULL;
> Error *local_err = NULL;
>
> + /*
> + * Caution: direct use of non-string @options members is
> + * problematic. When they come from -blockdev or blockdev_add,
> + * members are typed according to the QAPI schema, but when they
> + * come from -drive, they're all QString.
> + */
> drvname = qdict_get_try_str(*options, "driver");
Again, wordsmithing might be nice to call out that 'driver' is safe, but
future additions of other accesses must be careful.
> @@ -1987,6 +2000,12 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict
> *parent_options,
> qdict_extract_subqdict(parent_options, &options, bdref_key_dot);
> g_free(bdref_key_dot);
>
> + /*
> + * Caution: direct use of non-string @parent_options members is
> + * problematic. When they come from -blockdev or blockdev_add,
> + * members are typed according to the QAPI schema, but when they
> + * come from -drive, they're all QString.
> + */
> reference = qdict_get_try_str(parent_options, bdref_key);
Again, wordsmithing to mention that bdref_key is safe.
> if (reference || qdict_haskey(options, "file.filename")) {
> backing_filename[0] = '\0';
> @@ -2059,6 +2078,12 @@ bdrv_open_child_bs(const char *filename, QDict
> *options, const char *bdref_key,
> qdict_extract_subqdict(options, &image_options, bdref_key_dot);
> g_free(bdref_key_dot);
>
> + /*
> + * Caution: direct use of non-string @options members is
> + * problematic. When they come from -blockdev or blockdev_add,
> + * members are typed according to the QAPI schema, but when they
> + * come from -drive, they're all QString.
> + */
> reference = qdict_get_try_str(options, bdref_key);
Ditto.
> if (!filename && !reference && !qdict_size(image_options)) {
> if (!allow_none) {
> @@ -2275,8 +2300,11 @@ static BlockDriverState *bdrv_open_inherit(const char
> *filename,
> }
>
> /* Set the BDRV_O_RDWR and BDRV_O_ALLOW_RDWR flags.
> - * FIXME: we're parsing the QDict to avoid having to create a
> - * QemuOpts just for this, but neither option is optimal. */
> + * Caution: direct use of non-string @options members is
> + * problematic. When they come from -blockdev or blockdev_add,
> + * members are typed according to the QAPI schema, but when they
> + * come from -drive, they're all QString.
> + */
> if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_READ_ONLY), "on") &&
> !qdict_get_try_bool(options, BDRV_OPT_READ_ONLY, false)) {
> flags |= (BDRV_O_RDWR | BDRV_O_ALLOW_RDWR);
Maybe here, the wordsmithing would mention that the extra hoops we jump
through to cover both types is what makes access safe.
> +++ b/block/nbd.c
> @@ -278,6 +278,14 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict
> *options, Error **errp)
> goto done;
> }
>
> + /*
> + * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive
> + * server.type=inet. .to doesn't matter, it's ignored anyway.
> + * That's because when @options come from -blockdev or
> + * blockdev_add, members are typed according to the QAPI schema,
> + * but when they come from -drive, they're all QString. The
> + * visitor expects the former.
> + */
This one is fine.
> iv = qobject_input_visitor_new(crumpled_addr);
> visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
> if (local_err) {
> diff --git a/block/nfs.c b/block/nfs.c
> index 3f43f6e..42407de 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -474,6 +474,13 @@ static NFSServer *nfs_config(QDict *options, Error
> **errp)
> goto out;
> }
>
> + /*
> + * Caution: this works only because all scalar members of
> + * NFSServer are QString in @crumpled_addr. The visitor expects
> + * @crumpled_addr to be typed according to the QAPI scherma. It
> + * is when @options come from -blockdev or blockdev_add. But when
> + * they come from -drive, they're all QString.
> + */
> iv = qobject_input_visitor_new(crumpled_addr);
This one is also fine.
> visit_type_NFSServer(iv, NULL, &server, &local_error);
> if (local_error) {
> diff --git a/block/rbd.c b/block/rbd.c
> index 498322b..b9a9526 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -384,6 +384,12 @@ static int qemu_rbd_create(const char *filename,
> QemuOpts *opts, Error **errp)
> goto exit;
> }
>
> + /*
> + * Caution: direct use of non-string @options members is
> + * problematic. When they come from -blockdev or blockdev_add,
> + * members are typed according to the QAPI schema, but when they
> + * come from -drive, they're all QString.
Here, wordsmithing may be worth mentioning that we are safe because
these are all strings.
> + */
> pool = qdict_get_try_str(options, "pool");
> conf = qdict_get_try_str(options, "conf");
> clientname = qdict_get_try_str(options, "user");
> diff --git a/block/ssh.c b/block/ssh.c
> index 278e66f..471ba8a 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -601,6 +601,14 @@ static InetSocketAddress *ssh_config(QDict *options,
> Error **errp)
> goto out;
> }
>
> + /*
> + * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive.
> + * .to doesn't matter, it's ignored anyway.
> + * That's because when @options come from -blockdev or
> + * blockdev_add, members are typed according to the QAPI schema,
> + * but when they come from -drive, they're all QString. The
> + * visitor expects the former.
> + */
> iv = qobject_input_visitor_new(crumpled_addr);
This one's fine.
The overall idea makes sense, but since this is still RFC, and there may
still be wordsmithing to do, I'll wait to give R-b until v3.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
