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

Re: [libvirt] [PATCH] build: add compiler attributes to virUUIDParse

On 10/13/2011 02:55 AM, Daniel Veillard wrote:
On Wed, Oct 12, 2011 at 05:27:40PM -0600, Eric Blake wrote:
Coverity complained that most, but not all, clients of virUUIDParse
were checking for errors.  Silence those coverity warnings by
explicitly marking the cases where we trust the input, and fixing
one instance that really should have been checking.  In particular,
this silences about half of the 46 warnings I saw on my most
recent Coverity analysis run.

@@ -129,9 +129,6 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid) {
      const char *cur;
      int i;

-    if ((uuidstr == NULL) || (uuid == NULL))
-        return(-1);

   ACK, but I'm always a bit  distressed at the idea that a perfectly
   valid runtime check is being replaced by a static one which is
   compiler specific. Can we keep this chunk without Coverity complaining ?

Unfortunately, no. Keeping this code will make Coverity warn about dead code (the ATTRIBUTE_NONNULL implies that the check for NULL will always fail). All callers were already passing valid pointers; directly passing NULL will make gcc warn, and there's an open BZ against gcc to make it someday do the same analysis as Coverity to warn about any other obvious passing of NULL. But in spite of gcc's weakness, both clang and coverity catch a lot more cases of passing unintentional NULL, and get run frequently enough that I'm not too worried about dropping the argument check (it's an internal function, so we should be calling it correctly in the first place).

   But fine, ACK

I've gone ahead and pushed this.

Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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