Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 7/7/20 7:48 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> 
>>> Coverity noticed commit 950c4e6c94 introduced a dereference before
>>> null check in get_opt_value (CID1391003):
>>>
>>>   In get_opt_value: All paths that lead to this null pointer
>>>   comparison already dereference the pointer earlier (CWE-476)
>>>
>>> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
>>> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
>>> for the 'value' parameter".
>>>
>>> Since this function is publicly exposed, it risks new users to do
>>> the same error again. Avoid that documenting the 'value' argument
>>> must not be NULL.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> v2: Drop confuse comment (Damien Hedde)
>>> ---
>>>  include/qemu/option.h | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>>> index eb4097889d..ac50d25774 100644
>>> --- a/include/qemu/option.h
>>> +++ b/include/qemu/option.h
>>> @@ -28,6 +28,19 @@
>>>  
>>>  #include "qemu/queue.h"
>>>  
>>> +/**
>>> + * get_opt_value
>>> + * @p: a pointer to the option name, delimited by commas
>>> + * @value: a non-NULL pointer that will received the delimited options
>> 
>> s/received/receive/
>> 
>>> + *
>>> + * The @value char pointer will be allocated and filled with
>>> + * the delimited options.
>>> + *
>>> + * Returns the position of the comma delimiter/zero byte after the
>>> + * option name in @p.
>>> + * The memory pointer in @value must be released with a call to g_free()
>>> + * when no longer required.
>>> + */
>>>  const char *get_opt_value(const char *p, char **value);
>>>  
>>>  void parse_option_size(const char *name, const char *value,
>> 
>> You are adding a *second* doc comment: the definition already has one.
>> It's clearer than yours on some things, and less explicit on others.
>> Feel free to improve or replace it.  But do put it next to the
>> definition.
>
> Hmm I haven't noticed it, because my reflex is to look at the usage
> description in the prototype declaration, not in the implementation.
>
> I know, 2 different schools.
>
> Maybe we can make both schools less unhappy by simply duplicating the
> function description in both the header and the source files...

Worst of both worlds :)

[...]