Re: [Qemu-devel] [PATCH v6 02/23] qapi: Require int64/uint64 implementation

[Eric Blake]

On 11/27/2015 05:05 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Now that all visitors supply both type_int64() and type_uint64()
>> callbacks, we can drop the redundant type_int() callback (the
>> public interface visit_type_int() remains, but calls into
>> type_int64() under the hood).
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>

>>  void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error 
>> **errp)
>>  {
>> -    int64_t value;
>> +    uint64_t value;
>>
>>      if (v->type_uint8) {
>>          v->type_uint8(v, obj, name, errp);
>>      } else {
>>          value = *obj;
>> -        v->type_int(v, &value, name, errp);
>> -        if (value < 0 || value > UINT8_MAX) {
>> +        v->type_uint64(v, &value, name, errp);
>> +        if (value > UINT8_MAX) {
>>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>                         name ? name : "null", "uint8_t");
>>              return;
> 
> Note that this relies on value being in range after type_uint64() fails.
> If it isn't, we call error_setg() with non-null *errp.
> 
> Two solutions:
> 
> 1. Stipulate that type_uint64() & friends leave value alone on error.
>    Works, because its initial value *obj is in range.

Pre-existing and simpler, but sets a poor example for the rest of the
code base (not everyone is going to read the fine print for why it works
here), and requires cross-file audits to ensure visitors comply.

> 
> 2. Avoid using value on error.  A clean way to do this:
> 
>         Error *err = NULL;
> 
>         value = *obj;
>         v->type_uint64(v, &value, name, &err);
>         if (err) {
>             error_propagate(errp, err);
>             return;
>         }
>         if (value < 0 || value > UINT8_MAX) {
>             error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                        name ? name : "null", "uint8_t");
>             return;
>         }
>         *obj = value;
> 
>    More boilerplate.  If we pick this solution, we'll want a separate
>    PATCH 1.5 cleaning up the preexisting instances.

Of course, if I do the cleanup as 1.5, then patch 3/23 reindents
everything, that's a lot of churn.  So I may end up rearranging 2 and 3
after all, and then do the cleanup as 3.5.

Or maybe option 3, write a pair of helper functions containing the
boilerplate for checking against min and max:

void visit_type_intN(Visitor *v, int64_t *obj, const char *name,
                     int64_t min, int64_t max, Error **errp);
void visit_type_uintN(Visitor *v, int64_t *obj, const char *name,
                      uint64_t max, Error **errp);

leaving us with simpler clients:

visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
{
    int64_t value = *obj;
    visit_type_uintN(v, &value, name, INT8_MIN, INT8_MAX, errp);
    *obj = value;
}

and here, because the helpers are in the same file, it's easier to prove
that value was unchanged on error.  Or I may even squash 2 and 3 into a
single patch now.

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

signature.asc
Description: OpenPGP digital signature