| Message ID | 1541156463-2730-1-git-send-email-liq3ea@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: misc: change the 'pc' to unsinged 64 in CpuInfo | expand |
On 11/2/18 6:01 AM, Li Qiang wrote: > When trigger a 'query-cpus' qmp, the pc is an signed value like > following: > {"arch": "x86", ... "pc": -1732653994, "halted": true,...} > It is strange. Change it to uint64_t. > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > --- > qapi/misc.json | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) I don't see this as causing any major backwards-incompatible behavior to clients that can parse full 64-bit unsigned numbers (note that not all JSON parsers do so - here's frowning at you, jansson - but libvirt is okay). Reviewed-by: Eric Blake <eblake@redhat.com>
On 2 November 2018 at 11:01, Li Qiang <liq3ea@gmail.com> wrote: > When trigger a 'query-cpus' qmp, the pc is an signed value like > following: > {"arch": "x86", ... "pc": -1732653994, "halted": true,...} > It is strange. Change it to uint64_t. > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > --- NB: typo in subject line: should be "unsigned". thanks -- PMM
Eric Blake <eblake@redhat.com> writes: > On 11/2/18 6:01 AM, Li Qiang wrote: >> When trigger a 'query-cpus' qmp, the pc is an signed value like >> following: >> {"arch": "x86", ... "pc": -1732653994, "halted": true,...} >> It is strange. Change it to uint64_t. >> >> Signed-off-by: Li Qiang <liq3ea@gmail.com> >> --- >> qapi/misc.json | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) > > I don't see this as causing any major backwards-incompatible behavior > to clients that can parse full 64-bit unsigned numbers (note that not > all JSON parsers do so - here's frowning at you, jansson - but libvirt > is okay). > > Reviewed-by: Eric Blake <eblake@redhat.com> Client breakage is possible, but feels unlikely. To break, a client has to (a) accept negative numbers modulo 2^64, and (b) don't accept numbers >= 2^63. That's extra work. Clients that simply use strtoul() are unaffected. We accepted this very risk when we fixed the QObject visitor for unsigned integers in commit 5923f85fb82. Relevant part of the commit message: Note: before the patch, uint64_t values above INT64_MAX are sent over json QMP as negative values, e.g. UINT64_MAX is sent as -1. After the patch, they are sent unmodified. Clearly a bug fix, but we have to consider compatibility issues anyway. libvirt should cope fine, because its parsing of unsigned integers accepts negative values modulo 2^64. There's hope that other clients will, too. This patch needs to discuss the risk, too. Suggest to steal from the commit message I quoted. There's another question: 'uint64' or 'size'? Short answer: I think we better stick to 'uint64' here. Long answer follows. Both QAPI types map to uint64_t in C. The difference is twofold: * It serves as a documentation of intent in the QAPI schema. Or rather it would, if we used it more consistently. Related: [RFC PATCH 00/56] qapi: Use 'size' for byte counts & offsets https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01089.html Message-Id: <1502117160-24655-1-git-send-email-armbru@redhat.com> From its cover letter: Byte sizes, offsets and the like should use QAPI type 'size' (uint64_t). This rule is more honored in the breach than in the observance. Fix the obvious offenders. The series doesn't fix the program counters in the CpuInfoFOO. * A few visitors treat 'size' differently: - The opts visitor parses with qemu_strtosz() instead of parse_uint(), and it doesn't support ranges. Let's ignore ranges. qemu_strtosz() accepts convenience syntax like 1.5M for 1.5 * 1024 * 1024. This visitor is used for parsing CLI options -numa, -netdev, -object, -acpitable, HMP netdev_add, object_add, and the -object equivalents of qemu-img, qemu-io, qemu-nbd. - The QObject input visitor's keyval flavour parses with qemu_strtosz() instead of qemu_strtou64(). This visitor is used for parsing CLI options -display (except for its legacy forms), -blockdev, and by block layer in ways that are too complex to explain here. - The string input visitor[*] parses with qemu_strtosz() (via parse_option_size()) instead of qemu_strtou64(), and it doesn't support ranges. This visitor is used for parsing HMP migrate_set_parameter and QOM property values. It's also used for getting QOM property values (don't ask, it's terrible). - The string output visitor uses format PRIu64 instead of PRId64 and optionally PRIx64 as well, and it doesn't support ranges. This visitor is used for HMP info migrate, info memdev, info network, info qtree. It's also used for getting QOM property values (terrible). Yes, this stuff could have used adult design supervision. As far as I can tell, none of the 'size' differences is relevant for this patch. So, on the one hand, program counters are byte counts, so 'size' feels more appropriate. On the other hand, the important part is changing to unsigned, and 'uint64' gets us there without the additional baggage of 'size'. The additional baggage looks irrelevant for this patch. But perhaps we should make up our mind on usage of 'size' in general. The true reason it exists is the desire for convenience syntax at the human interface. Fair enough, but it doesn't scale: we could use convenience syntax in many other places, but creating a special type for each kind is an awful way to get it. If we had a better, more general way to specify "use this convenience syntax for human interfaces" in the schema, we could get rid of size. [*] With David Hildenbrand's patches applied.
diff --git a/qapi/misc.json b/qapi/misc.json index 6c1c5c0a37..621ec6ce13 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -407,7 +407,7 @@ # # Since: 2.6 ## -{ 'struct': 'CpuInfoX86', 'data': { 'pc': 'int' } } +{ 'struct': 'CpuInfoX86', 'data': { 'pc': 'uint64' } } ## # @CpuInfoSPARC: @@ -420,7 +420,7 @@ # # Since: 2.6 ## -{ 'struct': 'CpuInfoSPARC', 'data': { 'pc': 'int', 'npc': 'int' } } +{ 'struct': 'CpuInfoSPARC', 'data': { 'pc': 'uint64', 'npc': 'uint64' } } ## # @CpuInfoPPC: @@ -431,7 +431,7 @@ # # Since: 2.6 ## -{ 'struct': 'CpuInfoPPC', 'data': { 'nip': 'int' } } +{ 'struct': 'CpuInfoPPC', 'data': { 'nip': 'uint64' } } ## # @CpuInfoMIPS: @@ -442,7 +442,7 @@ # # Since: 2.6 ## -{ 'struct': 'CpuInfoMIPS', 'data': { 'PC': 'int' } } +{ 'struct': 'CpuInfoMIPS', 'data': { 'PC': 'uint64' } } ## # @CpuInfoTricore: @@ -453,7 +453,7 @@ # # Since: 2.6 ## -{ 'struct': 'CpuInfoTricore', 'data': { 'PC': 'int' } } +{ 'struct': 'CpuInfoTricore', 'data': { 'PC': 'uint64' } } ## # @CpuInfoRISCV: @@ -464,7 +464,7 @@ # # Since 2.12 ## -{ 'struct': 'CpuInfoRISCV', 'data': { 'pc': 'int' } } +{ 'struct': 'CpuInfoRISCV', 'data': { 'pc': 'uint64' } } ## # @CpuS390State:
When trigger a 'query-cpus' qmp, the pc is an signed value like following: {"arch": "x86", ... "pc": -1732653994, "halted": true,...} It is strange. Change it to uint64_t. Signed-off-by: Li Qiang <liq3ea@gmail.com> --- qapi/misc.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)