Re: [Qemu-devel] [RFC PATCH 00/56] qapi: Use 'size' for byte counts & offsets

[Kevin Wolf <kwolf@redhat.com>]

Am 07.08.2017 um 16:45 hat Markus Armbruster geschrieben:
> 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 is RFC for at least two reasons:
> 
> 1. It's only lightly tested.  Commit message claims like "FOO now
>    works" haven't been verified, yet.
> 
> 2. The block layer represents file offsets and sizes as int64_t in
>    many places.  Must not be negative, except for function return
>    values, where it means failure.
> 
>    If you pass negative values via QMP, things explode.  Rejecting
>    them cleanly would be better, but we do that only haphazardly, as
>    far as I can tell.
> 
>    Changing the QAPI schema from 'int' (C: int64_t) to 'size' (C:
>    uint64_t) arguably makes things slightly worse: you can't
>    reasonably expect negative offsets and sizes to work, but expecting
>    offsets and sizes between 2^63 and 2^64-1 to work is less
>    unreasonable.  The explosions stay the same.
> 
>    Perhaps we should have a dedicated QAPI type for file offsets, just
>    like libc has off_t.  Which is also signed, by the way.

I discussed this a bit on IRC with Markus and I'll just reply here with
some of our findings:

* Having a 63-bit unsigned type makes sense. Not because it's the most
  accurate type for most places and will catch all invalid arguments,
  it's probably still too large in most places and individual function
  needs to keep (or finally introduce) checks for the valid range. But
  compared to 'int' it doesn't allow us to forget the < 0 check, and
  compared to 'uint64' the resulting values are immune to careless
  casting between unsigned and signed C types. These seem to be common
  bugs, so getting rid of them would be nice.

* 'size' is the right type for sizes, offsets, etc. but the problem is
  likely to affect other arguments, too. 'size' enables additional
  syntax in the string visitor, so it is different from the other
  integer types. This means that we probably want both a 63 bit size
  type and a 63 bit plain unsigned integer type.

* 'int' is an alias for (signed) 'int64'. People don't seem to think
  much about using 'int' because it's the simplest type. That doesn't
  make it right to use, though. It may be better to remove the 'int'
  type and force any definitions to be specific about the signedness and
  width they need. My intuitive guess is that most places that use 'int'
  today don't actually want to accept negative numbers.

* Schema introspection doesn't distinguish between integer types, this
  is purely internal, so changing a definition from one integer type to
  another is okay.

Markus, did I forget anything important?

Kevin