Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> On Mon, Jul 29, 2013 at 7:15 PM, Markus Armbruster <address@hidden> wrote:
>> Luiz Capitulino <address@hidden> writes:
>>
>>> On Fri, 19 Jul 2013 04:57:51 -0600
>>> Eric Blake <address@hidden> wrote:
>>>
>>>> On 07/18/2013 08:36 PM, Pawit Pornkitprasan wrote:
>>>> > The qmp_migrate method uses the 'blk' and 'inc' parameter without
>>>> > checking if they're valid or not (they may be uninitialized if
>>>> > command is received via QMP)
>>>> >
>>>> > Signed-off-by: Pawit Pornkitprasan <address@hidden>
>>>> > ---
>>>> >  migration.c | 4 ++--
>>>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> Reviewed-by: Eric Blake <address@hidden>
>>>>
>>>> However, wouldn't it be nice if we improved the qapi generator to
>>>> guarantee a sane default value for optional parameters, even when
>>>> has_value is false?
>>>
>>> We could do that for bool and pointers, but this wouldn't help
>>> integers and enums. Also, even if we had default values, I guess
>>> I'd enforce a common idiom for handling optionals as this is also
>>> a good practice for preventing bugs.
>>
>> I disagree on pointers.
>>
>> "have_ptr && ptr->..." is a stupid idiom.  The common idiom for safe
>> dereference is "ptr && ptr->...".
>>
>> "bool have_ptr, FOO *ptr" in a parameter list is unidiomatic C.
>> Idiomatic is just "FOO *ptr".
>>
>> Conciseness is a virtue.
>>
>> For non-pointers, there's no special "don't have one" value, so we'd
>> have to declare a default value in the schema to get rid of the
>> have_FOO.
>
> The problem is the protocol.  There are cases where the absence of a
> value is different than having a default value for the parameter.
>
> This was part of the discussion way back when this all was first
> introduced.  Since everything was open coded and we had to preserve
> the semantics, that was the only choice we had.

Yes, there is a difference between an optional parameter with a default
value and one without.

If you got a value QMP clients cannot send, the difference can be
eliminated: just pass this value to stand for "nothing".  This is the
case for pointers: just pass NULL.  Like everybody else does when
passing "nothing".

When "nothing" is to be interpreted just like a context-independent
default value, then the difference doesn't matter.  The client doesn't
care, and the function handling the message becomes simpler.

Only when "nothing" can mean different things do you really need both
have_FOO and FOO parameters.

I can't see why we couldn't introduce default values into the schema if
we wanted, and use them to simplify generated code.

For pointers, we don't even need schema extensions to simplify away the
silly have_FOOs.