[Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws

Markus Armbruster posted 10 patches 6 years, 9 months ago
Failed in applying to current master (apply log)
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
blockdev.c                              |  14 +++++
docs/devel/qapi-code-gen.txt            |  10 +--
hmp.c                                   |  88 ++++++++++++--------------
hw/ppc/spapr_drc.c                      |   4 +-
include/qapi/qmp/qobject.h              |  10 ++-
include/qapi/visitor-impl.h             |   3 +-
include/qapi/visitor.h                  |   8 +--
include/qemu/typedefs.h                 |   1 +
migration/migration.c                   |  96 +++++++++++++++++++++++++---
qapi-schema.json                        | 108 ++++++++++++++++++++++++++++----
qapi/block-core.json                    |  29 ++++++---
qapi/qapi-clone-visitor.c               |   5 +-
qapi/qapi-dealloc-visitor.c             |   6 +-
qapi/qapi-visit-core.c                  |   7 ++-
qapi/qobject-input-visitor.c            |   6 +-
qapi/qobject-output-visitor.c           |   5 +-
qapi/string-input-visitor.c             |   8 ++-
qapi/string-output-visitor.c            |   3 +-
qapi/trace-events                       |   2 +-
qobject/json-parser.c                   |   2 +-
qobject/qnull.c                         |   8 ++-
scripts/qapi.py                         |   5 +-
target/i386/cpu.c                       |   4 +-
target/ppc/translate_init.c             |   5 +-
tests/check-qjson.c                     |   6 +-
tests/check-qnull.c                     |  27 ++++----
tests/qapi-schema/qapi-schema-test.json |   3 +-
tests/qapi-schema/qapi-schema-test.out  |   1 +
tests/qemu-iotests/085                  |   2 +-
tests/qemu-iotests/139                  |   2 +-
tests/test-qobject-input-visitor.c      |  24 ++++---
tests/test-qobject-output-visitor.c     |  13 +++-
32 files changed, 378 insertions(+), 137 deletions(-)
[Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws
Posted by Markus Armbruster 6 years, 9 months ago
blockdev-add and migrate-set-parameters overload empty strings to mean
something entirely different.  See my memo "qapi: Stop abusing
"special" values for something entirely different" for details.

    Message-ID: <87379zhrhn.fsf@dusky.pond.sub.org>
    https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04526.html

This series deprecates these usages of "" in favour of JSON null.
Because we're so close to the 2.10 freeze, the implementation is
intentionally stupid: rewrite null to "" at first opportunity.  The
proper way to do it would be rewriting "" to null, but that requires
fixing up code to work with null.  There are TODO comments for that.
I'm willing to take care of them in the next development cycle.

v2, because my pull request clashed with Juan's:
* Rebased, non-trivial conflicts in migration/migration.c
* PATCH 02: Commit message typo [Eric]
  	    Fix ppc [patchew]
* PATCH 03: qapi-code-gen.txt update [Eric]
* PATCH 09: Conflict resolution, somewhat ugly
* PATCH 10: Doc improvement [Eric]
  	    Conflict resolution

Markus Armbruster (10):
  qapi: Separate type QNull from QObject
  qapi: Use QNull for a more regular visit_type_null()
  qapi: Introduce a first class 'null' type
  tests/test-qobject-input-visitor: Drop redundant test
  block: Use JSON null instead of "" to disable backing file
  hmp: Clean up and simplify hmp_migrate_set_parameter()
  migration: Clean up around tls_creds, tls_hostname
  migration: Add TODO comments on duplication of QAPI_CLONE()
  migration: Unshare MigrationParameters struct for now
  migration: Use JSON null instead of "" to reset parameter to default

 blockdev.c                              |  14 +++++
 docs/devel/qapi-code-gen.txt            |  10 +--
 hmp.c                                   |  88 ++++++++++++--------------
 hw/ppc/spapr_drc.c                      |   4 +-
 include/qapi/qmp/qobject.h              |  10 ++-
 include/qapi/visitor-impl.h             |   3 +-
 include/qapi/visitor.h                  |   8 +--
 include/qemu/typedefs.h                 |   1 +
 migration/migration.c                   |  96 +++++++++++++++++++++++++---
 qapi-schema.json                        | 108 ++++++++++++++++++++++++++++----
 qapi/block-core.json                    |  29 ++++++---
 qapi/qapi-clone-visitor.c               |   5 +-
 qapi/qapi-dealloc-visitor.c             |   6 +-
 qapi/qapi-visit-core.c                  |   7 ++-
 qapi/qobject-input-visitor.c            |   6 +-
 qapi/qobject-output-visitor.c           |   5 +-
 qapi/string-input-visitor.c             |   8 ++-
 qapi/string-output-visitor.c            |   3 +-
 qapi/trace-events                       |   2 +-
 qobject/json-parser.c                   |   2 +-
 qobject/qnull.c                         |   8 ++-
 scripts/qapi.py                         |   5 +-
 target/i386/cpu.c                       |   4 +-
 target/ppc/translate_init.c             |   5 +-
 tests/check-qjson.c                     |   6 +-
 tests/check-qnull.c                     |  27 ++++----
 tests/qapi-schema/qapi-schema-test.json |   3 +-
 tests/qapi-schema/qapi-schema-test.out  |   1 +
 tests/qemu-iotests/085                  |   2 +-
 tests/qemu-iotests/139                  |   2 +-
 tests/test-qobject-input-visitor.c      |  24 ++++---
 tests/test-qobject-output-visitor.c     |  13 +++-
 32 files changed, 378 insertions(+), 137 deletions(-)

-- 
2.7.5


Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/10] Correct two minor QMP interface design flaws
Posted by John Snow 6 years, 8 months ago
Looks like this series got no replies (maybe a failure of my mail
filtering?) but it has since been merged, so purely for my own selfish
purposes;

Merged upstream as:

01fa55982692fb51a16049b63b571651a1053989 migration: Use JSON null
instead of "" to reset parameter to default
1bda8b3c6950f74482ba19e8529db72b511ba977 migration: Unshare
MigrationParameters struct for now
e87fae4c488ff8a10b921311a63f16b94031c611 migration: Add TODO comments on
duplication of QAPI_CLONE()
8cc99dcdc264bc896926e43e7576c7b7ab633d70 migration: Clean up around
tls_creds, tls_hostname
7e91e82044f8d23acfb9949a2cdd667a6b239acd hmp: Clean up and simplify
hmp_migrate_set_parameter()
c42e8742f527476839bcc5f91c3d2ea456ca6a45 block: Use JSON null instead of
"" to disable backing file
06f80154b2ada1d58ac504e98ff6e943b069b96c
tests/test-qobject-input-visitor: Drop redundant test
4d2d5c41a9e8ee201cda8be8701f7f9fc92e71aa qapi: Introduce a first class
'null' type
d2f95f4d482374485234790a6fc3cca29ebb7355 qapi: Use QNull for a more
regular visit_type_null()
006ca09f3027d86346fce707e9295975c6558f42 qapi: Separate type QNull from
QObject

--js

On 07/20/2017 03:53 AM, Markus Armbruster wrote:
> blockdev-add and migrate-set-parameters overload empty strings to mean
> something entirely different.  See my memo "qapi: Stop abusing
> "special" values for something entirely different" for details.
> 
>     Message-ID: <87379zhrhn.fsf@dusky.pond.sub.org>
>     https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04526.html
> 
> This series deprecates these usages of "" in favour of JSON null.
> Because we're so close to the 2.10 freeze, the implementation is
> intentionally stupid: rewrite null to "" at first opportunity.  The
> proper way to do it would be rewriting "" to null, but that requires
> fixing up code to work with null.  There are TODO comments for that.
> I'm willing to take care of them in the next development cycle.
> 

Did this get dropped in favor of a more comprehensive 2.11 fix?

> v2, because my pull request clashed with Juan's:
> * Rebased, non-trivial conflicts in migration/migration.c
> * PATCH 02: Commit message typo [Eric]
>   	    Fix ppc [patchew]
> * PATCH 03: qapi-code-gen.txt update [Eric]
> * PATCH 09: Conflict resolution, somewhat ugly
> * PATCH 10: Doc improvement [Eric]
>   	    Conflict resolution
> 
> Markus Armbruster (10):
>   qapi: Separate type QNull from QObject
>   qapi: Use QNull for a more regular visit_type_null()
>   qapi: Introduce a first class 'null' type
>   tests/test-qobject-input-visitor: Drop redundant test
>   block: Use JSON null instead of "" to disable backing file
>   hmp: Clean up and simplify hmp_migrate_set_parameter()
>   migration: Clean up around tls_creds, tls_hostname
>   migration: Add TODO comments on duplication of QAPI_CLONE()
>   migration: Unshare MigrationParameters struct for now
>   migration: Use JSON null instead of "" to reset parameter to default
> 
>  blockdev.c                              |  14 +++++
>  docs/devel/qapi-code-gen.txt            |  10 +--
>  hmp.c                                   |  88 ++++++++++++--------------
>  hw/ppc/spapr_drc.c                      |   4 +-
>  include/qapi/qmp/qobject.h              |  10 ++-
>  include/qapi/visitor-impl.h             |   3 +-
>  include/qapi/visitor.h                  |   8 +--
>  include/qemu/typedefs.h                 |   1 +
>  migration/migration.c                   |  96 +++++++++++++++++++++++++---
>  qapi-schema.json                        | 108 ++++++++++++++++++++++++++++----
>  qapi/block-core.json                    |  29 ++++++---
>  qapi/qapi-clone-visitor.c               |   5 +-
>  qapi/qapi-dealloc-visitor.c             |   6 +-
>  qapi/qapi-visit-core.c                  |   7 ++-
>  qapi/qobject-input-visitor.c            |   6 +-
>  qapi/qobject-output-visitor.c           |   5 +-
>  qapi/string-input-visitor.c             |   8 ++-
>  qapi/string-output-visitor.c            |   3 +-
>  qapi/trace-events                       |   2 +-
>  qobject/json-parser.c                   |   2 +-
>  qobject/qnull.c                         |   8 ++-
>  scripts/qapi.py                         |   5 +-
>  target/i386/cpu.c                       |   4 +-
>  target/ppc/translate_init.c             |   5 +-
>  tests/check-qjson.c                     |   6 +-
>  tests/check-qnull.c                     |  27 ++++----
>  tests/qapi-schema/qapi-schema-test.json |   3 +-
>  tests/qapi-schema/qapi-schema-test.out  |   1 +
>  tests/qemu-iotests/085                  |   2 +-
>  tests/qemu-iotests/139                  |   2 +-
>  tests/test-qobject-input-visitor.c      |  24 ++++---
>  tests/test-qobject-output-visitor.c     |  13 +++-
>  32 files changed, 378 insertions(+), 137 deletions(-)
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/10] Correct two minor QMP interface design flaws
Posted by Eric Blake 6 years, 8 months ago
On 08/16/2017 04:34 PM, John Snow wrote:
> Looks like this series got no replies (maybe a failure of my mail
> filtering?) but it has since been merged, so purely for my own selfish
> purposes;

No replies to v2 due to the fact that the differences from v1 were
trivial enough to keep R-b from that spin (it was mostly conflict
resolution that required the respin).

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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/10] Correct two minor QMP interface design flaws
Posted by John Snow 6 years, 8 months ago

On 08/16/2017 05:41 PM, Eric Blake wrote:
> On 08/16/2017 04:34 PM, John Snow wrote:
>> Looks like this series got no replies (maybe a failure of my mail
>> filtering?) but it has since been merged, so purely for my own selfish
>> purposes;
> 
> No replies to v2 due to the fact that the differences from v1 were
> trivial enough to keep R-b from that spin (it was mostly conflict
> resolution that required the respin).
> 

That's fine; just very visibly marking it as "done" for the purposes of
the list, archives, tooling, etc.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/10] Correct two minor QMP interface design flaws
Posted by Markus Armbruster 6 years, 8 months ago
John Snow <jsnow@redhat.com> writes:

[...]
> On 07/20/2017 03:53 AM, Markus Armbruster wrote:
>> blockdev-add and migrate-set-parameters overload empty strings to mean
>> something entirely different.  See my memo "qapi: Stop abusing
>> "special" values for something entirely different" for details.
>> 
>>     Message-ID: <87379zhrhn.fsf@dusky.pond.sub.org>
>>     https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04526.html
>> 
>> This series deprecates these usages of "" in favour of JSON null.
>> Because we're so close to the 2.10 freeze, the implementation is
>> intentionally stupid: rewrite null to "" at first opportunity.  The
>> proper way to do it would be rewriting "" to null, but that requires
>> fixing up code to work with null.  There are TODO comments for that.
>> I'm willing to take care of them in the next development cycle.
>> 
>
> Did this get dropped in favor of a more comprehensive 2.11 fix?

This is the stupid solution for 2.10.  TODO comments in PATCH 05+08-10
mark the spots that could use cleanup.  I hope to get to them in 2.11.

[...]