Re: [libvirt] [PATCH 6/7] xend_internal.c: assure clang that we do not dereference NULL
Eric Blake wrote:
On 04/14/2010 10:02 AM, Jim Meyering wrote:
> From: Jim Meyering <meyering(a)redhat.com>
>
> * src/xen/xend_internal.c (xend_parse_sexp_desc_char): Add three
> uses of sa_assert, each preceding a strchr(value,... to assure
> clang that "value" is non-NULL.
> ---
> src/xen/xend_internal.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index c4e73b7..950f1b5 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -1284,6 +1284,7 @@ xend_parse_sexp_desc_char(virBufferPtr buf,
> virBufferVSprintf(buf, " <source
path='%s'/>\n",
> value);
> } else if (STREQ(type, "tcp")) {
> + sa_assert (value);
> const char *offset = strchr(value, ':');
This introduces an expression before a declaration, even when sa_assert
is empty. I know that we already rely on several other C99 features
(like __VA_ARGS__ from the preprocessor), but my understanding has been
that we have been trying to stick with C89 declarations first still.
Does this need to be refactored accordingly?
There have been others like this, and no one objected then.
Prohibiting stmt-after-decl is detrimental.
Take this example. Prohibiting it here would change perfectly
usable/readable code into this longer and slightly inferior equivalent:
(inferior because it's longer and hence detracts from overall readability,
and because "offset" is now duplicated)
const char *offset;
sa_assert (value);
offset = strchr(value, ':');
Besides, xend_parse_sexp_desc_char already dereferences value at
line
1224;
True, but just after that, it may be set to NULL:
if (value[0] == '/') {
type = "dev";
} else if (STRPREFIX(value, "null")) {
type = "null";
value = NULL; <<<<===================
} else if (STRPREFIX(value, "vc")) {
...
and *that* is the case clang complains about.
would it be possible to fix this by marking the argument as
NONNULL, rather than adding sa_assert()?
No.