Max Reitz <mre...@redhat.com> writes: > On 29.03.2017 18:45, Markus Armbruster wrote: >> -blockdev and blockdev_add convert their arguments via QObject to >> BlockdevOptions for qmp_blockdev_add(), which converts them back to >> QObject, then to a flattened QDict. The QDict's members are typed >> according to the QAPI schema. >> >> -drive converts its argument via QemuOpts to a (flat) QDict. This >> QDict's members are all QString. >> >> Thus, the QType of a flat QDict member depends on whether it comes >> from -drive or -blockdev/blockdev_add, except when the QAPI type maps >> to QString, which is the case for 'str' and enumeration types. >> >> The block layer core extracts generic configuration from the flat >> QDict, and the block driver extracts driver-specific configuration. >> >> Both commonly do so by converting (parts of) the flat QDict to >> QemuOpts, which turns all values into strings. Not exactly elegant, >> but correct. >> >> 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> >> --- >> block.c | 41 +++++++++++++++++++++++++++++++++++++++-- >> block/file-posix.c | 6 ++++++ >> block/nbd.c | 8 ++++++++ >> block/nfs.c | 7 +++++++ >> block/rbd.c | 6 ++++++ >> block/ssh.c | 8 ++++++++ >> 6 files changed, 74 insertions(+), 2 deletions(-) > > I have to say I don't like how the comments in block.c are completely > generic.
I'm afraid they reflect my rather "generic" understanding of block.c. >> diff --git a/block.c b/block.c >> index 6e906ec..b3ce23f 100644 >> --- a/block.c >> +++ 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"); > > For instance this one: Well, yes, for -drive, this will always be a > QString. Which is OK, because that's what we're trying to get. > > The comment makes this confusing, IMO. If you really want a comment here > it should at least contain a mention that it's totally fine in practice > here. Calling the code "problematic" sounds like this could blow up when > it reality it can't; and I would think it actually is the most sane > solution given the current state of the whole infrastructure (i.e. how > -drive and -blockdev work). Well, if it could blow up, I'd call it wrong, and start the comment with FIXME :) Even though qdict_get_try_str() is indeed fine, I propose to have a comment, because someone with less detailed understanding of how the configuration machine works (me, until yesterday, and probably again after next month) could conclude that qdict_get_try_bool() would be just as fine. What about: /* * Caution: while qdict_get_try_str() is fine, getting non-string * types would require more care. When @options come from -blockdev * or blockdev_add, its members are typed according to the QAPI * schema, but when they come from -drive, they're all QString. */ If you don't like this comment either, care to suggest one you do? > Same for all the other plain qdict_get_try_str() calls (in block.c, at > least). > >> } >> >> @@ -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"); >> if (drvname) { >> drv = bdrv_find_format(drvname); >> @@ -1358,6 +1370,7 @@ static int bdrv_fill_options(QDict **options, const >> char *filename, >> } >> >> /* Find the right block driver */ >> + /* Direct use of @options members is problematic, see note above */ >> filename = qdict_get_try_str(*options, "filename"); >> >> if (!drvname && protocol) { >> @@ -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); >> 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); >> 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. >> + */ > > Now this one should mention that this is the very reason why we are > attempting parsing the QDict string before getting a boolean value directly. What about: /* * Set the BDRV_O_RDWR and BDRV_O_ALLOW_RDWR flags. * Caution: getting a boolean member of @options requires care. * 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. */ >> 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); >> @@ -2298,6 +2326,7 @@ static BlockDriverState *bdrv_open_inherit(const char >> *filename, >> options = qdict_clone_shallow(options); >> >> /* Find the right image format driver */ >> + /* Direct use of @options members is problematic, see note above */ >> drvname = qdict_get_try_str(options, "driver"); >> if (drvname) { >> drv = bdrv_find_format(drvname); >> @@ -2309,6 +2338,7 @@ static BlockDriverState *bdrv_open_inherit(const char >> *filename, >> >> assert(drvname || !(flags & BDRV_O_PROTOCOL)); >> >> + /* Direct use of @options members is problematic, see note above */ >> backing = qdict_get_try_str(options, "backing"); >> if (backing && *backing == '\0') { >> flags |= BDRV_O_NO_BACKING; >> @@ -2787,6 +2817,13 @@ int bdrv_reopen_prepare(BDRVReopenState >> *reopen_state, BlockReopenQueue *queue, >> do { >> QString *new_obj = qobject_to_qstring(entry->value); >> const char *new = qstring_get_str(new_obj); >> + /* >> + * Caution: direct use of non-string bs->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. >> + */ >> const char *old = qdict_get_try_str(reopen_state->bs->options, >> entry->key); >> >> diff --git a/block/file-posix.c b/block/file-posix.c >> index 0841a08..738541e 100644 >> --- a/block/file-posix.c >> +++ b/block/file-posix.c >> @@ -2193,6 +2193,12 @@ static int hdev_open(BlockDriverState *bs, QDict >> *options, int flags, >> int ret; >> >> #if defined(__APPLE__) && defined(__MACH__) >> + /* >> + * 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. >> + */ >> const char *filename = qdict_get_str(options, "filename"); > > This comment I'm very much OK with, though, because we should not > retrieve runtime options directly from the QDict. > > (Same for the comments in the other block drivers.) > > Max > >> char bsd_path[MAXPATHLEN] = ""; >> bool error_occurred = false;