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 <mre...@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 <ebl...@redhat.com> Please let me keep it :-) Max
signature.asc
Description: OpenPGP digital signature
