[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/7] string-input-visitor: Fix uint64 parsing
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 1/7] string-input-visitor: Fix uint64 parsing |
Date: |
Wed, 30 Sep 2015 07:47:05 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
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