[libvirt] [PATCH] storage_backend.c: assure clang that inputvol can't be NULL
clang was complaining that a NULL inputvol would be dereferenced in that "could not open..." diagnostic. Since the two sole callers of this function are careful to call it only when inputvol is non-NULL, this is a good case for giving the parameter the nonnull attribute:
From 314278acb021a1f2e63494fab352bd6e9a4a38bb Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 3 Sep 2009 10:22:57 +0200 Subject: [PATCH] storage_backend.c: assure clang that inputvol can't be NULL
* src/storage_backend.c (virStorageBackendCopyToFD): Declare inputvol parameter to be "nonnull". Remove test for non-NULL inputvol. Both callers ensure it's non-NULL. --- src/storage_backend.c | 16 +++++++--------- 1 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/storage_backend.c b/src/storage_backend.c index c818142..8d1187e 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -1,7 +1,7 @@ /* * storage_backend.c: internal storage driver backend contract * - * Copyright (C) 2007-2008 Red Hat, Inc. + * Copyright (C) 2007-2009 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -106,7 +106,7 @@ enum { static int virStorageBackendCopyToFD(virConnectPtr conn, virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol, + virStorageVolDefPtr inputvol ATTRIBUTE_NONNULL, int fd, unsigned long long *total, int is_dest_file) @@ -119,13 +119,11 @@ virStorageBackendCopyToFD(virConnectPtr conn, char zerobuf[512]; char *buf = NULL; - if (inputvol) { - if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { - virReportSystemError(conn, errno, - _("could not open input path '%s'"), - inputvol->target.path); - goto cleanup; - } + if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { + virReportSystemError(conn, errno, + _("could not open input path '%s'"), + inputvol->target.path); + goto cleanup; } bzero(&zerobuf, sizeof(zerobuf)); -- 1.6.4.2.395.ge3d52
On Thu, Sep 03, 2009 at 11:18:00AM +0200, Jim Meyering wrote:
clang was complaining that a NULL inputvol would be dereferenced in that "could not open..." diagnostic.
Since the two sole callers of this function are careful to call it only when inputvol is non-NULL, this is a good case for giving the parameter the nonnull attribute:
ACK. BTW, how exactly are you getting all these warnings/diagnostics ? Are you merely compiling libvirt using clang & just collecting the warnings, or is this some special static analysis tool ? If we're going to start adding these non-null attributes, then I should hook the appropriate tool into autobuild.sh to validate them regularly. 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 :|
Daniel P. Berrange wrote:
On Thu, Sep 03, 2009 at 11:18:00AM +0200, Jim Meyering wrote:
clang was complaining that a NULL inputvol would be dereferenced in that "could not open..." diagnostic.
Since the two sole callers of this function are careful to call it only when inputvol is non-NULL, this is a good case for giving the parameter the nonnull attribute:
ACK.
BTW, how exactly are you getting all these warnings/diagnostics ? Are you merely compiling libvirt using clang & just collecting the warnings, or is this some special static analysis tool ?
If we're going to start adding these non-null attributes, then I should hook the appropriate tool into autobuild.sh to validate them regularly.
Definitely. I built llvm/clang per instructions here: http://clang-analyzer.llvm.org/ Than ran them like this: scan-build -o clang ./autogen.sh scan-build -o clang make That produces a bunch of html in the -o-specified dierctory, clang/. Point your browser at clang/*/index.html to view the results.
Daniel P. Berrange wrote:
On Thu, Sep 03, 2009 at 11:18:00AM +0200, Jim Meyering wrote:
clang was complaining that a NULL inputvol would be dereferenced in that "could not open..." diagnostic.
Since the two sole callers of this function are careful to call it only when inputvol is non-NULL, this is a good case for giving the parameter the nonnull attribute:
When I recompiled everything, this patch needed adjustment. For example, I had to include internal.h for the definition of ATTRIBUTE_NONNULL.
From 20229d1c5cb35221cc72ec37f4204e5f80cda4a9 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 3 Sep 2009 10:22:57 +0200 Subject: [PATCH 1/4] storage_backend.c: assure clang that inputvol can't be NULL
* src/storage_backend.c: Include "internal.h". (virStorageBackendCopyToFD): Mark inputvol parameter as "nonnull". Remove test for non-NULL inputvol. Both callers ensure it's non-NULL. --- src/storage_backend.c | 17 ++++++++--------- 1 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/storage_backend.c b/src/storage_backend.c index c818142..77243ef 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -1,7 +1,7 @@ /* * storage_backend.c: internal storage driver backend contract * - * Copyright (C) 2007-2008 Red Hat, Inc. + * Copyright (C) 2007-2009 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -47,6 +47,7 @@ #include "util.h" #include "memory.h" #include "node_device.h" +#include "internal.h" #include "storage_backend.h" @@ -103,7 +104,7 @@ enum { TOOL_QCOW_CREATE, }; -static int +static int ATTRIBUTE_NONNULL (3) virStorageBackendCopyToFD(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, @@ -119,13 +120,11 @@ virStorageBackendCopyToFD(virConnectPtr conn, char zerobuf[512]; char *buf = NULL; - if (inputvol) { - if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { - virReportSystemError(conn, errno, - _("could not open input path '%s'"), - inputvol->target.path); - goto cleanup; - } + if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { + virReportSystemError(conn, errno, + _("could not open input path '%s'"), + inputvol->target.path); + goto cleanup; } bzero(&zerobuf, sizeof(zerobuf)); -- 1.6.4.2.395.ge3d52
On Thu, Sep 03, 2009 at 12:49:25PM +0200, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Thu, Sep 03, 2009 at 11:18:00AM +0200, Jim Meyering wrote:
clang was complaining that a NULL inputvol would be dereferenced in that "could not open..." diagnostic.
Since the two sole callers of this function are careful to call it only when inputvol is non-NULL, this is a good case for giving the parameter the nonnull attribute:
When I recompiled everything, this patch needed adjustment. For example, I had to include internal.h for the definition of ATTRIBUTE_NONNULL.
ACK thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering