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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list