On Wed, Sep 02, 2009 at 11:48:21AM -0400, Jim Paris wrote:
Yeah, for functions where it is expected that the passed in param be non-NULL, then annotations are definitely the way togo. This lets the compiler/checkers validate the callers instead, avoiding the need to clutter the methods with irrelevant NULL checks.
Does __attribute__((__nonnull__())) really cover the case we're concerned about here? As I understand it, it tells the compiler
1) if the caller obviously passes NULL, emit a warning 2) assume that the arguments are non-NULL and optimize away
In other words it will do nothing to prevent a null dereference and will even make it more likely since non-NULL checks will be skipped.
We should only add the nonnull attribute annotation on methods where we are not intending to do any checks for non-NULL. They would already be segfaulting if passed a NULL pointer, so adding the annotation gives us the static validation of the existing non-NULL assumption.
For example:
test.c: -------- #include <stdio.h> #include <stdlib.h>
void foo(int *a) { if (a) printf("%d\n", *a); else printf("(null)\n"); }
__attribute__((__nonnull__ (1))) void bar(int *a) { if (a) printf("%d\n", *a); else printf("(null)\n"); }
int main(void) { foo(malloc(1000000000000ULL)); bar(malloc(1000000000000ULL)); return 0; } --------
$ gcc -O2 -Wall -Werror -o test test.c $ ./test (null) Segmentation fault
THis is not a case where we would add a non-null annotation - the method is clearly expecting a NULL parameter as valid since it has a check & branch to handle that. So using the annotation is not correct. The prime example of usefulness in libvirt is in the main API glue - In include/libvirt.h most of the parameters are documented to be non-NULL. We should *not*, however, annotate libvirt.h since the implmentation of these APIs is including checks for NULL and returns a runtime error to the caller in this case. We don't want the compiler to optimize away these checks - In the src/driver.h, and the implementations of these driver APIs eg qemu_driver.c, xen_unified.c, we *should* include the non-NULL annotation. These methods are relying on the fact that libvirt.c has already weeded out any NULL pointers and so don't do any NULL checks themselves. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|