Re: [libvirt] [PATCH (RFC?)] Remove usage of __attribute__((nonnull))

On Tue, Mar 28, 2017 at 01:46:31PM +0200, Martin Kletzander wrote: Actually the attribute does both and the primary intention of the attribute *is* build time warnings and/or static analysis warnings: [quote] 'nonnull (ARG-INDEX, ...)' The 'nonnull' attribute specifies that some function parameters should be non-null pointers. For instance, the declaration: extern void * my_memcpy (void *dest, const void *src, size_t len) __attribute__((nonnull (1, 2))); causes the compiler to check that, in calls to 'my_memcpy', arguments DEST and SRC are non-null. If the compiler determines that a null pointer is passed in an argument slot marked as non-null, and the '-Wnonnull' option is enabled, a warning is issued. The compiler may also choose to make optimizations based on the knowledge that certain function arguments will never be null. [/quote] IIRC, the problem we had was that we marked some methods as non-null, but still passed NULL values into them. The compiler had optimized away the code paths that if 'if (arg == NULL) { return 0 }', meaning the later '*arg' de-reference would fail. The use of nonnull attribute would help the compiler report mistakes, but the compiler only catches some easy cases at build time. So the key issue is whether we have enough confidence in fact that our calling methods really will be passing a non-NULL value or not. If we're not confident in our callers for a method, then we should remove nonnull, and have an explicit 'if (arg == NULL)' check in the code instead. Some projects might use asserts() against args being non-NULL, but libvirt tends to try to return soft errors. Given this, it could be better use to 'if (arg == NULL)' checks in general, over nonnull, unless we have high confidence in callers. Screws them up in what way ? Do we not use '-DSTATIC_ANLYSIS' when running through coverity, so that coverity sees the nonnull annotations and thus is able to do more advanced checks for NULL args ? If so, removing it would make coverity miss bugs. If we do decide to remove ATTRIBUTE_NONNULL annotations, then we need to make sure the method impls have suitable 'if (args == NULL) ....' checks that handle this safely. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|