Re: [Qemu-devel] [RFC v2 for-2.9 04/10] block: Document -drive problematic code and bugs

[Eric Blake]

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 <address@hidden>
> ---

> +++ 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