[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] QAPI 'size' vs. 'uint64' (was: [PATCH] qapi: misc: change t
From: |
Markus Armbruster |
Subject: |
[Qemu-devel] QAPI 'size' vs. 'uint64' (was: [PATCH] qapi: misc: change the 'pc' to unsinged 64 in CpuInfo) |
Date: |
Thu, 29 Nov 2018 16:38:26 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>> 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 <address@hidden>
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: <address@hidden>
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.