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

[Markus Armbruster]

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.

I'm not trying to re-argue where to put doc comments.  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.