Re: [Qemu-devel] [PATCH 1/7] string-input-visitor: Fix uint64 parsing

[Eric Blake]

On 09/30/2015 07:19 AM, Markus Armbruster wrote:

> 
> The (essentially undocumented) Visitor abstraction has the following
> methods for integers:

I proposed documentation at:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05434.html

> 
> * Mandatory: type_int()
> 
>   Interface uses int64_t for the value.  The implementation should
>   ensure it fits into int64_t.
> 
> * Optional: type_int{8,16,32}()
> 
>   These use int{8,16,32}_t for the value.
> 
>   If present, it should ensure the value fits into the data type.
> 
>   If missing, the core falls back to type_int() plus appropriate range
>   checking.

No one implements them.  In fact, as part of preparing my documentation,
I actually proposed simplifying the visitor callback interface to drop them:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05432.html

> 
> * Optional: type_int64()
> 
>   Same interface as type_int().
> 
>   If present, it should ensure the value fits into int64_t.
> 
>   If missing, the core falls back to type_int().
> 
>   Aside: setting type_int64() would be useful only when you want to
>   distinguish QAPI types int and int64.  So far, nobody does.  In fact,
>   nobody uses QAPI type int64!  I'm tempted to define QAPI type int as a
>   mere alias for int64 and drop the redundant stuff.

Already part of my proposal.

> 
> * Optional: type_uint{8,16,32}()
> 
>   These use uint{8,16,32}_t for the value.
> 
>   If present, it should ensure the value fits into the data type.
> 
>   If missing, the core falls back to type_int() plus appropriate range
>   checking.

Also unused, and simplified above.

> 
> * Optional: type_uint64()
> 
>   Now it gets interesting.  Interface uses uint64_t for the value.
> 
>   If present, it should ensure the value fits into uint64_t.
> 
>   If missing, the core falls back to type_int().  No range checking.  If
>   type_int() performs range checking as it should, then uint64_t values
>   not representable in int64_t get rejected (wrong), and negative values
>   representable in int64_t get cast to uint64_t (also wrong).
> 
>   I think we need to make type_uint64() mandatory, and drop the
>   fallback.

Probably a good idea, although not done in my proposed patches.

> 
> * Optional: type_size()
> 
>   Same interface as type_uint64().
> 
>   If present, it should ensure the value fits into uint64_t.
> 
>   If missing, the core first tries falling back to type_uint64() and
>   then to type_int().  Falling back to type_int() is as wrong here as it
>   is in type_uint64().

Provided by the QemuOpts parser to allow '1k' to mean 1024, and so on.

> 
>> As a bug fix, ignore warnings about preference of qemu_strto[u]ll().
> 
> I'm not sure I get this sentence.
> 
>> Cc: address@hidden
>> Signed-off-by: Andreas Färber <address@hidden>
> 
> On the actual patch, I have nothing to add over Eric's review right now.
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature