[Qemu-devel] [PATCH v6 0/6] block: Don't compare strings in bdrv_reopen_prepare()

Max Reitz posted 6 patches 6 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171004152553.30263-1-mreitz@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
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            | 325 +++++++++++++++++++++++++++++++++++++++
scripts/coccinelle/qobject.cocci |   3 +
tests/.gitignore                 |   1 +
tests/qemu-iotests/133           |   9 ++
tests/qemu-iotests/133.out       |   5 +
25 files changed, 589 insertions(+), 26 deletions(-)
create mode 100644 include/qapi/qmp/qnull.h
create mode 100644 tests/check-qobject.c
[Qemu-devel] [PATCH v6 0/6] block: Don't compare strings in bdrv_reopen_prepare()
Posted by Max Reitz 6 years, 5 months ago
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.


v6:
- Patch 2: Added corresponding Coccinelle rule [Eric]
- Patch 6:
  - Added .gitignore entry [Eric]
  - Removed leading underscore [Markus]
  - Replaced test_equality() by check_equal()+check_unequal() [Markus]
  - Put type conversion (or rather non-conversion) tests into an own
    function [Markus]
  - Fixed a line > 80 characters


git-backport-diff against v5:

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:[0001] [FC] '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:[0097] [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            | 325 +++++++++++++++++++++++++++++++++++++++
 scripts/coccinelle/qobject.cocci |   3 +
 tests/.gitignore                 |   1 +
 tests/qemu-iotests/133           |   9 ++
 tests/qemu-iotests/133.out       |   5 +
 25 files changed, 589 insertions(+), 26 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h
 create mode 100644 tests/check-qobject.c

-- 
2.13.6


Re: [Qemu-devel] [PATCH v6 0/6] block: Don't compare strings in bdrv_reopen_prepare()
Posted by Markus Armbruster 6 years, 4 months ago
Max Reitz <mreitz@redhat.com> writes:

> 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.

Series looks ready to me.  It touches QAPI to achieve its purpose in the
block layer; I'd be fine with merging it via a block tree.

Re: [Qemu-devel] [PATCH v6 0/6] block: Don't compare strings in bdrv_reopen_prepare()
Posted by Max Reitz 6 years, 4 months ago
On 2017-11-10 10:16, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> 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.
> 
> Series looks ready to me.  It touches QAPI to achieve its purpose in the
> block layer; I'd be fine with merging it via a block tree.

Thanks!  So now it's my problem to figure out whether this is 2.11
material...? :-)

Max

Re: [Qemu-devel] [PATCH v6 0/6] block: Don't compare strings in bdrv_reopen_prepare()
Posted by Kevin Wolf 6 years, 4 months ago
Am 10.11.2017 um 18:36 hat Max Reitz geschrieben:
> On 2017-11-10 10:16, Markus Armbruster wrote:
> > Max Reitz <mreitz@redhat.com> writes:
> > 
> >> 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.
> > 
> > Series looks ready to me.  It touches QAPI to achieve its purpose in the
> > block layer; I'd be fine with merging it via a block tree.
> 
> Thanks!  So now it's my problem to figure out whether this is 2.11
> material...? :-)

The test case in patch 5 segfaults without the series. Why would it not
be a bug fix (= 2.11 material)?

Kevin
Re: [Qemu-devel] [PATCH v6 0/6] block: Don't compare strings in bdrv_reopen_prepare()
Posted by Max Reitz 6 years, 4 months ago
On 2017-11-10 18:47, Kevin Wolf wrote:
> Am 10.11.2017 um 18:36 hat Max Reitz geschrieben:
>> On 2017-11-10 10:16, Markus Armbruster wrote:
>>> Max Reitz <mreitz@redhat.com> writes:
>>>
>>>> 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.
>>>
>>> Series looks ready to me.  It touches QAPI to achieve its purpose in the
>>> block layer; I'd be fine with merging it via a block tree.
>>
>> Thanks!  So now it's my problem to figure out whether this is 2.11
>> material...? :-)
> 
> The test case in patch 5 segfaults without the series. Why would it not
> be a bug fix (= 2.11 material)?

Because it adds a whole lot of QAPI code, and the segfault is a clear
NULL pointer dereference which you can only get through HMP.  But then
again, the only place where the new code is used is from the place of
the bug fix itself, so I guess no regressions are possible.

So if it is a bug fix, why have you not applied it? :-)


Applied to my block branch for 2.11:

https://github.com/XanClic/qemu/commits/block

Max

Re: [Qemu-devel] [PATCH v6 0/6] block: Don't compare strings in bdrv_reopen_prepare()
Posted by Max Reitz 6 years, 4 months ago
On 2017-10-04 17:25, 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.
> 
> 
> v6:
> - Patch 2: Added corresponding Coccinelle rule [Eric]
> - Patch 6:
>   - Added .gitignore entry [Eric]
>   - Removed leading underscore [Markus]
>   - Replaced test_equality() by check_equal()+check_unequal() [Markus]
>   - Put type conversion (or rather non-conversion) tests into an own
>     function [Markus]
>   - Fixed a line > 80 characters

Dropping from my queue because of a clang compiler warning in
tests/check-qobject.c.

Max