Eric Blake <ebl...@redhat.com> writes:
> 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..."
I haven't updated this patch for review, yet. One more reason this is
RFC. Forgot to mention in the cover letter, sorry.
>> }
>>
>> @@ -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.
Yes.
>> @@ -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.
Yes.
>> 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.
Yes.
>> 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.
Yes.
>> +++ 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.
Yes.
>> + */
>> 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.
Thanks!
