bdrv_reopen_prepare() assumes that all BDS options are strings, which is
not necessarily correct. This series introduces a new qobject_is_equal()
function which can be used to test whether any options have changed,
independently of their type.


v7:
- Patch 6: Fix a clang warning:
    tests/check-qobject.c:39:24: error: passing an object that undergoes
                                 default argument promotion to
                                 'va_start' has undefined behavior
  TIL: You cannot use va_start(ap, foo) if @foo is a bool.  An int
       works, however.
       Feel free to explain the long version to me, because I don't
       think I have fully understood the issue, but it's something like
       "Using bools for variadic arguments results in their promotion to
       an int, but you have to use a type that is promoted to itself
       (like int)."


git-backport-diff against v6:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/6:[----] [--] 'qapi/qnull: Add own header'
002/6:[----] [--] 'qapi/qlist: Add qlist_append_null() macro'
003/6:[----] [--] 'qapi: Add qobject_is_equal()'
004/6:[----] [--] 'block: qobject_is_equal() in bdrv_reopen_prepare()'
005/6:[----] [--] 'iotests: Add test for non-string option reopening'
006/6:[0011] [FC] 'tests: Add check-qobject for equality tests'


Max Reitz (6):
  qapi/qnull: Add own header
  qapi/qlist: Add qlist_append_null() macro
  qapi: Add qobject_is_equal()
  block: qobject_is_equal() in bdrv_reopen_prepare()
  iotests: Add test for non-string option reopening
  tests: Add check-qobject for equality tests

 tests/Makefile.include           |   4 +-
 include/qapi/qmp/qbool.h         |   1 +
 include/qapi/qmp/qdict.h         |   2 +
 include/qapi/qmp/qlist.h         |   4 +
 include/qapi/qmp/qnull.h         |  32 ++++
 include/qapi/qmp/qnum.h          |   1 +
 include/qapi/qmp/qobject.h       |  21 ++-
 include/qapi/qmp/qstring.h       |   1 +
 include/qapi/qmp/types.h         |   1 +
 block.c                          |  29 ++--
 qapi/qapi-clone-visitor.c        |   1 +
 qapi/string-input-visitor.c      |   1 +
 qobject/qbool.c                  |   8 +
 qobject/qdict.c                  |  29 ++++
 qobject/qlist.c                  |  32 ++++
 qobject/qnull.c                  |  11 +-
 qobject/qnum.c                   |  54 +++++++
 qobject/qobject.c                |  29 ++++
 qobject/qstring.c                |   9 ++
 tests/check-qnull.c              |   2 +-
 tests/check-qobject.c            | 328 +++++++++++++++++++++++++++++++++++++++
 scripts/coccinelle/qobject.cocci |   3 +
 tests/.gitignore                 |   1 +
 tests/qemu-iotests/133           |   9 ++
 tests/qemu-iotests/133.out       |   5 +
 25 files changed, 592 insertions(+), 26 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h
 create mode 100644 tests/check-qobject.c

-- 
2.13.6

On 11/14/2017 12:01 PM, Max Reitz wrote:
> bdrv_reopen_prepare() assumes that all BDS options are strings, which is
> not necessarily correct. This series introduces a new qobject_is_equal()
> function which can be used to test whether any options have changed,
> independently of their type.
> 
> 
> v7:
> - Patch 6: Fix a clang warning:
>     tests/check-qobject.c:39:24: error: passing an object that undergoes
>                                  default argument promotion to
>                                  'va_start' has undefined behavior
>   TIL: You cannot use va_start(ap, foo) if @foo is a bool.  An int
>        works, however.

I knew that va_arg(ap, bool) was undefined behavior, but didn't realize
va_start(ap, bool_param) was also undefined.  But sure enough, reading
C99 7.15.1.4:

4 The parameter parmN is the identifier of the rightmost parameter in
the variable
parameter list in the function definition (the one just before the ,
...). If the parameter
parmN is declared with the register storage class, with a function or
array type, or
with a type that is not compatible with the type that results after
application of the default
argument promotions, the behavior is undefined.

>        Feel free to explain the long version to me, because I don't
>        think I have fully understood the issue, but it's something like
>        "Using bools for variadic arguments results in their promotion to
>        an int, but you have to use a type that is promoted to itself
>        (like int)."

So it must be that C99 is trying to cater to platforms that have special
ABI for passing multiple bool on the stack, by stating that the moment
argument promotion is in effect, va_list adjacent to a bool may cause
problems with that special packing in the ABI.  Weird.

> 
> 
> git-backport-diff against v6:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/6:[----] [--] 'qapi/qnull: Add own header'
> 002/6:[----] [--] 'qapi/qlist: Add qlist_append_null() macro'
> 003/6:[----] [--] 'qapi: Add qobject_is_equal()'
> 004/6:[----] [--] 'block: qobject_is_equal() in bdrv_reopen_prepare()'
> 005/6:[----] [--] 'iotests: Add test for non-string option reopening'
> 006/6:[0011] [FC] 'tests: Add check-qobject for equality tests'

I still think this is 2.11 material.  Once you fix the typo I point out
separately on 6/6, the changes since v6 look reasonable, so series:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 2017-11-14 19:01, Max Reitz wrote:
> bdrv_reopen_prepare() assumes that all BDS options are strings, which is
> not necessarily correct. This series introduces a new qobject_is_equal()
> function which can be used to test whether any options have changed,
> independently of their type.

Aaand once again applied to my block branch.

(As always, thank you for reviewing, Eric -- I'm happy to have expanded
your knowledge on obscure C behavior. :-))

Max