Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL "conn"

Jim Paris wrote: That's true, in a way. Once a parameter is marked nonnull, tools can do a better job of spotting *callers* that might mistakenly pass a corresponding NULL argument. Without nonnull, the compiler could only detect a problem *inside* the function, in the case that we lack adequate protection on a dereferencing expression. For external functions, it's a policy decision. If you say the function is defined only for non-NULL pointers, then you may as well add an assert(ptr != NULL) and __attribute__((nonnull...)). Otherwise, we must perform the test at run-time. Note that memcpy, strcpy, unlink, stat, etc. have parameters that can be marked with the nonnull attribute. That's a plus, because compiler/analyzer tools can then warn callers about misuse, and they needn't be burdened with detecting and diagnosing NULL pointers. Using attribute-nonnull is not an excuse for skipping a "ptr == NULL" test. Rather, it is a way to tell the compiler that we require every caller to ensure that a "ptr" parameter is non-NULL. This has been true of most internal "conn" parameters for some time. There were even quite a few places in the code where "conn" would be dereferenced without first ensuring it is non-NULL, but there was no way to trigger the NULL-deref in those seemingly-erroneous bits of code, because upstream tests ensured non-NULL conn. I spent time adding guards, as I would find those disturbing bits of code. I now think that I should have been adding assertions and/or attribute-nonnull directives, instead. Adding an "assert (ptr != NULL);" is tempting, because then we'd get a pretty diagnostic with filename:lineno rather than just a segfault, whenever this assumption is violated. However, adding a raw "assert" use in library grade code is frowned upon, to say the least.