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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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...

> 
> I'm not trying to re-argue where to put doc comments.

We could, because the origin of both this patch and the commits
referenced that produced CID1391003.

> I *am* arguing
> for local consistency while we lack global consistency.  For code I
> maintain, I insist on local consistency.
> 
> The code belonging to MAINTAINERS section "Command line option argument
> parsing" has doc comments next to the definition.  Except for
> qemu_opt_has_help_opt(), which predates my maintainer mandate.
> 
Please disregard this patch, I don't mind about get_opt_value().