[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 6/7] xend_internal.c: assure clang that we do not dereference NULL



Eric Blake wrote:
> On 04/14/2010 11:29 AM, Jim Meyering wrote:
>>>>      } 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.
>
> In re-reading my comment, I can see my position was not clear.  I am
> very much in favor of relying on more C99 features, especially
> decl-after-stmt.  I'm not opposed to your solution, so much as
> questioning whether others are still trying to stick to the C89 rules,
> as well as point out that sticking to C89 in this case would be
> anachronistic given that we already have rely on other C99 features like
> // for comments.
>
>>> 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;      <<<<===================
>
> Ahh.  My review was not thorough enough, then.  Still, clang didn't
> complain about the first value[0] dereference?

Correct.  I looked a little further, and have perhaps discovered why.
That entire 230-line internal function is NEVER USED.  Not anywhere.
I've removed it, along with the decl in the .h file, and everything still links.

For now, I'll simply fix the objection from clang.
But the next step will be to remove it,
unless someone says they have plans for it.

> At any rate, your additional explanation, coupled with my second review
> of that function, is enough to warrant ACK, once 1/7 is resolved.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]