Eric Blake <ebl...@redhat.com> writes:
> On 11/27/2015 05:05 AM, Markus Armbruster wrote:
>> Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
>>>
>
>>> 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.
Yes, but "don't mess up externally visible state on error" is a pretty
common design maxim.
>> 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.
Artistic license applies.
