On 2017-07-05 09:14, Markus Armbruster wrote: > Max Reitz <[email protected]> writes: > >> Currently, bdrv_reopen_prepare() assumes that all BDS options are >> strings. However, this is not the case if the BDS has been created >> through the json: pseudo-protocol or blockdev-add. >> >> Note that the user-invokable reopen command is an HMP command, so you >> can only specify strings there. Therefore, specifying a non-string >> option with the "same" value as it was when originally created will now >> return an error because the values are supposedly similar (and there is >> no way for the user to circumvent this but to just not specify the >> option again -- however, this is still strictly better than just >> crashing). >> >> Signed-off-by: Max Reitz <[email protected]> >> --- >> block.c | 31 ++++++++++++++++++------------- >> 1 file changed, 18 insertions(+), 13 deletions(-) >> >> diff --git a/block.c b/block.c >> index 913bb43..45eb248 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2947,19 +2947,24 @@ int bdrv_reopen_prepare(BDRVReopenState >> *reopen_state, BlockReopenQueue *queue, >> const QDictEntry *entry = qdict_first(reopen_state->options); >> >> do { >> - QString *new_obj = qobject_to_qstring(entry->value); >> - const char *new = qstring_get_str(new_obj); >> - /* >> - * Caution: while qdict_get_try_str() is fine, getting >> - * non-string types would require more care. When >> - * bs->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. >> - */ >> - const char *old = qdict_get_try_str(reopen_state->bs->options, >> - entry->key); >> - >> - if (!old || strcmp(new, old)) { >> + QObject *new = entry->value; >> + QObject *old = qdict_get(reopen_state->bs->options, entry->key); >> + >> + /* TODO: When using -drive to specify blockdev options, all >> values >> + * will be strings; however, when using -blockdev, blockdev-add >> or >> + * filenames using the json:{} pseudo-protocol, they will be >> + * correctly typed. >> + * In contrast, reopening options are (currently) always strings >> + * (because you can only specify them through qemu-io; all other >> + * callers do not specify any options). >> + * Therefore, when using anything other than -drive to create a >> BDS, >> + * this cannot detect non-string options as unchanged, because >> + * qobject_is_equal() always returns false for objects of >> different >> + * type. In the future, this should be remedied by correctly >> typing >> + * all options. For now, this is not too big of an issue >> because >> + * the user simply can not specify options which cannot be >> changed >> + * anyway, so they will stay unchanged. */ > > I'm not the maintainer, and this is not a demand: consider winging this > comment and wrapping lines around column 70.
I actually did consider wrapping around column 70 and decided against it
because this comment is already indented by 12 characters, so none of
the lines exceed 65 characters (80 - (12 + strlen(" * "))).
About winging... For some reason I don't quite like it here. But it
probably is better because the comment is not immediately related to the
qobject_is_equal() call below, so I'll do it.
> As much as I fancy word play (see your reply to Eric), I have to admit
> that I had to read "the user simply can not specify options" about three
> times to make sense of it. Please consider a wording that's easier to
> grasp, perhaps "the user can simply refrain from specifying options that
> cannot be changed".
Aw. I've wanted this to be an example I could point people to to
educate them about the difference. Now, alas, none shall learn. :-(
Max
signature.asc
Description: OpenPGP digital signature
