| Message ID | 20170703122505.32017-4-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/03/2017 07:25 AM, Max Reitz wrote: > 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 s/invokable/invocable/ > 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 <mreitz@redhat.com> > --- > block.c | 31 ++++++++++++++++++------------- > 1 file changed, 18 insertions(+), 13 deletions(-) > > + /* 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 Seeing "can not" usually looks wrong for "cannot"; but here, it is grammatically correct. But better would be "the user can simply not specify" or "the user can simply avoid specifying options". > + * anyway, so they will stay unchanged. */ The grammar tweak is minor, so: Reviewed-by: Eric Blake <eblake@redhat.com>
On 2017-07-03 14:51, Eric Blake wrote: > On 07/03/2017 07:25 AM, Max Reitz wrote: >> 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 > > s/invokable/invocable/ I was asking this myself when writing this commit message and actually consulted Wiktionary: https://en.wiktionary.org/wiki/invokable >> 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 <mreitz@redhat.com> >> --- >> block.c | 31 ++++++++++++++++++------------- >> 1 file changed, 18 insertions(+), 13 deletions(-) >> > >> + /* 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 > > Seeing "can not" usually looks wrong for "cannot"; but here, it is > grammatically correct. But better would be "the user can simply not > specify" or "the user can simply avoid specifying options". Awww, I deliberately designed the sentence so I could use "can not". :-) (and even contrast it with the following "cannot"!) >> + * anyway, so they will stay unchanged. */ > > The grammar tweak is minor, so: > Reviewed-by: Eric Blake <eblake@redhat.com> Please let me keep it :-) Max
On 07/03/2017 08:01 AM, Max Reitz wrote: > On 2017-07-03 14:51, Eric Blake wrote: >> On 07/03/2017 07:25 AM, Max Reitz wrote: >>> 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 >> >> s/invokable/invocable/ > > I was asking this myself when writing this commit message and actually > consulted Wiktionary: https://en.wiktionary.org/wiki/invokable My intuitive reasoning for favoring 'c' is that we spell it "invocation", not "invokation", even if the short form is "invoke". But I also consulted dictionary.com before writing my original reply: http://www.dictionary.com/browse/invocable which pulls up "invoke" as the primary, and only lists "invocable" (and not "invokable") as the related form adjective. Meanwhile, you are correct that wiktionary does list both forms as alternates of one another, so I can live with the 'k' even though it looks odd. >>> + * all options. For now, this is not too big of an issue because >>> + * the user simply can not specify options which cannot be changed >> >> Seeing "can not" usually looks wrong for "cannot"; but here, it is >> grammatically correct. But better would be "the user can simply not >> specify" or "the user can simply avoid specifying options". > > Awww, I deliberately designed the sentence so I could use "can not". :-) > > (and even contrast it with the following "cannot"!) Maybe it's just that I'm being thrown off by the placement of 'simply'; as a native speaker, I have an easier time parsing "can simply not" than I do "simply can not", maybe because of where the emphasis of "simply" is falling. Which is perhaps weird, because in the infinitive form (coming up with a construct that uses "to" instead of "can"), I note that "it is possible to simply not specify options" is a split infinitive (which style guides tell you to avoid, even though I think it is acceptable); while "it is possible simply to not specify options" satisfies more style guides. But since you are intentionally doing it for the play on words, > >>> + * anyway, so they will stay unchanged. */ >> >> The grammar tweak is minor, so: >> Reviewed-by: Eric Blake <eblake@redhat.com> > Please let me keep it :-) you can keep it.
Max Reitz <mreitz@redhat.com> 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 <mreitz@redhat.com> > --- > 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. 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". > + if (!qobject_is_equal(new, old)) { > error_setg(errp, "Cannot change the option '%s'", entry->key); > ret = -EINVAL; > goto error;
On 2017-07-05 09:14, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> 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 <mreitz@redhat.com> >> --- >> 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
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. */ + if (!qobject_is_equal(new, old)) { error_setg(errp, "Cannot change the option '%s'", entry->key); ret = -EINVAL; goto error;
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 <mreitz@redhat.com> --- block.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)