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

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

 



On Thu, Oct 13, 2011 at 12:24:02PM -0600, Eric Blake wrote:
> 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).

Also note that if you have ATTRIBUTE_NONNULL, then GCC will actually
*remove* those two lines during optimization phase anyway. So by
leaving them you are giving yourself a misleading view of reality.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]