On 7/7/20 7:48 AM, Markus Armbruster wrote: > Philippe Mathieu-Daudé <phi...@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é <phi...@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().
