| Message ID | 1490805920-17029-5-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <armbru@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. > 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). 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. > 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;
Max Reitz <mreitz@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 <armbru@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;
On 03/30/2017 01:52 AM, Markus Armbruster wrote: >>> +++ 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. > */ Yes, that's better - it makes it obvious that our current usage works, but that the code must not be carelessly edited if we add another field in the future.
Eric Blake <eblake@redhat.com> writes: > On 03/30/2017 01:52 AM, Markus Armbruster wrote: > >>>> +++ 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. >> */ > > Yes, that's better - it makes it obvious that our current usage works, > but that the code must not be carelessly edited if we add another field > in the future. If Max is also happy with it, I'll put it in v3.
On 30.03.2017 16:42, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 03/30/2017 01:52 AM, Markus Armbruster wrote: >> >>>>> +++ 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. >>> */ >> >> Yes, that's better - it makes it obvious that our current usage works, >> but that the code must not be carelessly edited if we add another field >> in the future. > > If Max is also happy with it, I'll put it in v3. Yes, more than happy, thanks! (Same for the other comment.) Max
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"); } @@ -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. + */ 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"); char bsd_path[MAXPATHLEN] = ""; bool error_occurred = false; diff --git a/block/nbd.c b/block/nbd.c index 36ea617..11e3ba7 100644 --- a/block/nbd.c +++ 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. + */ 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); 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. + */ 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); visit_type_InetSocketAddress(iv, NULL, &inet, &local_error); if (local_error) {
-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 <armbru@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(-)