On Wed, Sep 02, 2009 at 12:26:43PM +0200, Jim Meyering wrote:
Daniel Veillard wrote:
On Wed, Sep 02, 2009 at 11:58:07AM +0200, Jim Meyering wrote:
Here's the test just before the else-if in the patch below:
if (conn && conn->driver && STREQ (conn->driver->name, "remote")) {
So, in the else-branch, "conn" is guaranteed to be NULL. And dereferenced.
This may be only a theoretical risk, but if so, the test of "conn" above should be changed to an assertion, and/or the parameter should get the nonnull attribute.
I'm fine with your patch, so that even if code around changes having a gard is IMHO a good thing
Thanks for the quick review. Daniel Berrange said he'd prefer the nonnull approach I mentioned above, so I've just adjusted (caveat, still not tested or even compiled)
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.
From 375bbb2d85fdf708858d726b6c7682e40dc07c2c Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 2 Sep 2009 12:20:32 +0200 Subject: [PATCH 1/2] infra: define ATTRIBUTE_NONNULL to mark non-NULL parameters
* src/internal.h (ATTRIBUTE_NONNULL): Define. --- src/internal.h | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/src/internal.h b/src/internal.h index 936cd03..8fa579c 100644 --- a/src/internal.h +++ b/src/internal.h @@ -116,6 +116,14 @@ #endif #endif
+#ifndef ATTRIBUTE_NONNULL +# if __GNUC_PREREQ (3, 3) +# define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m))) +# else +# define ATTRIBUTE_NONNULL(m) +# endif +#endif + #else #ifndef ATTRIBUTE_UNUSED #define ATTRIBUTE_UNUSED -- 1.6.4.2.395.ge3d52
From 61dc45fcae8b17d9f619563d427f1bc74f44d553 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 2 Sep 2009 12:22:14 +0200 Subject: [PATCH 2/2] remote_internal.c: appease clang
* src/remote_internal.c (remoteNetworkOpen): Mark "conn" parameter as non-NULL. Remove now-unnecessary "conn == NULL" test. (remoteDevMonOpen): Likewise. --- src/remote_internal.c | 10 ++++------ 1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/remote_internal.c b/src/remote_internal.c index ea50c11..fe36ddd 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -3281,13 +3281,12 @@ done: static virDrvOpenStatus remoteNetworkOpen (virConnectPtr conn, virConnectAuthPtr auth, - int flags) + int flags) ATTRIBUTE_NONNULL (1) { if (inside_daemon) return VIR_DRV_OPEN_DECLINED;
- if (conn && - conn->driver && + if (conn->driver && STREQ (conn->driver->name, "remote")) { struct private_data *priv;
@@ -5130,13 +5129,12 @@ done: static virDrvOpenStatus remoteDevMonOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - int flags ATTRIBUTE_UNUSED) + int flags ATTRIBUTE_UNUSED) ATTRIBUTE_NONNULL (1) { if (inside_daemon) return VIR_DRV_OPEN_DECLINED;
- if (conn && - conn->driver && + if (conn->driver && STREQ (conn->driver->name, "remote")) { struct private_data *priv = conn->privateData; /* If we're here, the remote driver is already
ACK 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 :|