[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function |
Date: |
Tue, 7 Jul 2020 08:11:20 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
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().