Re: [Qemu-trivial] [Qemu-devel] [PATCH v2] scripts: add sample model file for Coverity Scan

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Il 19/03/2014 18:32, Eric Blake ha scritto:
>>> + *
>>> + * Copyright (C) 2014 Red Hat, Inc.
>>> + *
>>> + * Authors:
>>> + *  Markus Armbruster <address@hidden>
>>> + *  Paolo Bonzini <address@hidden>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2
>>> or, at your
>>> + * option, any later version.  See the COPYING file in the
>>> top-level directory.
>>
>> Aren't the license and authors blurbs usually in the other order?
>
> Not in the sample I copied from (migration.c).
>
>>
>>> +#define assert(x) if (!(x)) __coverity_panic__();
>>
>> Will this break any 'if () assert(); else {}' blocks?  Obviously, such
>> blocks already violate coding convention, but you might as well make
>> this definition safe to use for older code.
>
> Ok.

Coverity's builtin model models assert already, see
library/generic/libc/all/all.c.  We shouldn't override it without a good
reason.

>>> +
>>> +static void __write(uint8_t *buf, int len)
>>
>> Will the fact that you used 'int len' instead of 'size_t' bite us on 32-
>> vs. 64-bit?  Same for __read.
>
> Yeah, I copied this from address_space_rw.  I'll change to ssize_t to
> catch negative values.

Change the real address_space_rw(), or the model's __write()?

>>> +void *
>>> +g_malloc0 (size_t n_bytes)
>>> +{
>>> +    void *mem;
>>> +    __coverity_negative_sink__((ssize_t) n_bytes);
>>> +    mem = calloc(1, n_bytes == 0 ? 1 : n_bytes);
>>> +    if (!mem) __coverity_panic__ ();
>>
>> Is it worth being consistent on spacing before (?
>
> Yes.
>
>>> +void g_free (void *mem)
>>> +{
>>> +    if (mem) {
>>> +        free(mem);
>>> +    }
>>
>> Doesn't coverity already know that free(NULL) is a no-op, without you
>> having to repeat it?
>
> This part came from Markus. :)

I don't know what possessed me when I wrote (or copied?) the silly
conditional.  Let's drop it before someone else copies it.