[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 05/21] Annotate many methods with ATTRIBUTE_RETURN_CHECK & fix problems



On Fri, Oct 23, 2009 at 02:05:34PM +0100, Daniel P. Berrange wrote:
> Nearly all of the methods in src/util/util.h have error codes that
> must be checked by the caller to correct detect & report failure.
> Add ATTRIBUTE_RETURN_CHECK to ensure compile time validation of
> this
> 
> * daemon/libvirtd.c: Add explicit check on return value of virAsprintf
> * src/conf/domain_conf.c: Add missing check on virParseMacAddr return
>   value status & report error
> * src/network/bridge_driver.c: Add missing OOM check on virAsprintf
>   and report error
> * src/qemu/qemu_conf.c: Add missing check on virParseMacAddr return
>   value status & report error
> * src/security/security_selinux.c: Remove call to virRandomInitialize
>   that's done in libvirt.c already
> * src/storage/storage_backend_logical.c: Add check & log on virRun
>   return status
> * src/util/util.c: Add missing checks on virAsprintf/Run status
> * src/util/util.h: Annotate all methods with ATTRIBUTE_RETURN_CHECK
>   if they return an error status code
> * src/vbox/vbox_tmpl.c: Add missing check on virParseMacAddr
> * src/xen/xm_internal.c: Add missing checks on virAsprintf
> * tests/qemuargv2xmltest.c: Remove bogus call to virRandomInitialize()
> ---
>  daemon/libvirtd.c                     |   13 ++++++---
>  src/conf/domain_conf.c                |    7 ++++-
>  src/network/bridge_driver.c           |    6 +++-
>  src/qemu/qemu_conf.c                  |    9 ++++++-
>  src/security/security_selinux.c       |    2 -
>  src/storage/storage_backend_logical.c |    5 +++-
>  src/util/util.c                       |    6 +++-
>  src/util/util.h                       |   44 ++++++++++++++++----------------
>  src/vbox/vbox_tmpl.c                  |    4 ++-
>  src/xen/xm_internal.c                 |    6 +++-
>  tests/qemuargv2xmltest.c              |    2 -
>  11 files changed, 64 insertions(+), 40 deletions(-)
> 
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 78dfb2d..fa473ce 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -752,13 +752,16 @@ static int qemudInitPaths(struct qemud_server *server,
>              goto snprintf_error;
>      }
>  
> -    if (server->privileged)
> -        server->logDir = strdup (LOCAL_STATE_DIR "/log/libvirt");
> -    else
> -        virAsprintf(&server->logDir, "%s/.libvirt/log", dir_prefix);
> +    if (server->privileged) {
> +        if (!(server->logDir = strdup (LOCAL_STATE_DIR "/log/libvirt")))
> +            virReportOOMError(NULL);
> +    } else {
> +        if (virAsprintf(&server->logDir, "%s/.libvirt/log", dir_prefix) < 0)
> +            virReportOOMError(NULL);
> +    }
>  
>      if (server->logDir == NULL)
> -        virReportOOMError(NULL);
> +        goto cleanup;
>  
>      ret = 0;
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0dd2b3f..a6d8e07 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1109,7 +1109,12 @@ virDomainNetDefParseXML(virConnectPtr conn,
>      }
>  
>      if (macaddr) {
> -        virParseMacAddr((const char *)macaddr, def->mac);
> +        if (virParseMacAddr((const char *)macaddr, def->mac) < 0) {
> +            virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                                 _("unable to parse mac address '%s'"),
> +                                 (const char *)macaddr);
> +            goto error;
> +        }
>      } else {
>          virCapabilitiesGenerateMac(caps, def->mac);
>      }
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 95bc810..2a6a7ab 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -149,7 +149,10 @@ networkFindActiveConfigs(struct network_driver *driver) {
>  #ifdef __linux__
>                  char *pidpath;
>  
> -                virAsprintf(&pidpath, "/proc/%d/exe", obj->dnsmasqPid);
> +                if (virAsprintf(&pidpath, "/proc/%d/exe", obj->dnsmasqPid) < 0) {
> +                    virReportOOMError(NULL);
> +                    goto cleanup;
> +                }
>                  if (virFileLinkPointsTo(pidpath, DNSMASQ) == 0)
>                      obj->dnsmasqPid = -1;
>                  VIR_FREE(pidpath);
> @@ -157,6 +160,7 @@ networkFindActiveConfigs(struct network_driver *driver) {
>              }
>          }
>  
> +    cleanup:
>          virNetworkObjUnlock(obj);
>      }
>  }
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 158e9a3..951a6c6 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -2841,7 +2841,14 @@ qemuParseCommandLineNet(virConnectPtr conn,
>      for (i = 0 ; i < nkeywords ; i++) {
>          if (STREQ(keywords[i], "macaddr")) {
>              genmac = 0;
> -            virParseMacAddr(values[i], def->mac);
> +            if (virParseMacAddr(values[i], def->mac) < 0) {
> +                qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                                 _("unable to parse mac address '%s'"),
> +                                 values[i]);
> +                virDomainNetDefFree(def);
> +                def = NULL;
> +                goto cleanup;
> +            }
>          } else if (STREQ(keywords[i], "model")) {
>              def->model = values[i];
>              values[i] = NULL;
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 7e0f71a..6a03af7 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -108,8 +108,6 @@ SELinuxInitialize(virConnectPtr conn)
>      char *ptr = NULL;
>      int fd = 0;
>  
> -    virRandomInitialize(time(NULL) ^ getpid());
> -
>      fd = open(selinux_virtual_domain_context_path(), O_RDONLY);
>      if (fd < 0) {
>          virReportSystemError(conn, errno,
> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
> index 4389120..eac3917 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -36,6 +36,7 @@
>  #include "storage_conf.h"
>  #include "util.h"
>  #include "memory.h"
> +#include "logging.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_STORAGE
>  
> @@ -341,7 +342,9 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn,
>       * that might be hanging around, so if this fails for some reason, the
>       * worst that happens is that scanning doesn't pick everything up
>       */
> -    virRun(conn, scanprog, &exitstatus);
> +    if (virRun(conn, scanprog, &exitstatus) < 0) {
> +        VIR_WARN0("Failure when running vgscan to refresh physical volumes");
> +    }
>  
>      memset(&sourceList, 0, sizeof(sourceList));
>      sourceList.type = VIR_STORAGE_POOL_LOGICAL;
> diff --git a/src/util/util.c b/src/util/util.c
> index 98f8a14..08070da 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -1257,7 +1257,8 @@ int virFileOpenTtyAt(const char *ptmx ATTRIBUTE_UNUSED,
>  char* virFilePid(const char *dir, const char* name)
>  {
>      char *pidfile;
> -    virAsprintf(&pidfile, "%s/%s.pid", dir, name);
> +    if (virAsprintf(&pidfile, "%s/%s.pid", dir, name) < 0)
> +        return NULL;
>      return pidfile;
>  }
>  
> @@ -2108,7 +2109,8 @@ void virFileWaitForDevices(virConnectPtr conn)
>       * If this fails for any reason, we still have the backup of polling for
>       * 5 seconds for device nodes.
>       */
> -    virRun(conn, settleprog, &exitstatus);
> +    if (virRun(conn, settleprog, &exitstatus) < 0)
> +    {}
>  }
>  #else
>  void virFileWaitForDevices(virConnectPtr conn ATTRIBUTE_UNUSED) {}
> diff --git a/src/util/util.h b/src/util/util.h
> index 8679636..3ef26e6 100644
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -41,8 +41,8 @@ enum {
>      VIR_EXEC_CLEAR_CAPS = (1 << 2),
>  };
>  
> -int virSetNonBlock(int fd);
> -int virSetCloseExec(int fd);
> +int virSetNonBlock(int fd) ATTRIBUTE_RETURN_CHECK;
> +int virSetCloseExec(int fd) ATTRIBUTE_RETURN_CHECK;
>  
>  /* This will execute in the context of the first child
>   * after fork() but before execve() */
> @@ -57,7 +57,7 @@ int virExecDaemonize(virConnectPtr conn,
>                       int flags,
>                       virExecHook hook,
>                       void *data,
> -                     char *pidfile);
> +                     char *pidfile) ATTRIBUTE_RETURN_CHECK;
>  int virExecWithHook(virConnectPtr conn,
>                      const char *const*argv,
>                      const char *const*envp,
> @@ -69,7 +69,7 @@ int virExecWithHook(virConnectPtr conn,
>                      int flags,
>                      virExecHook hook,
>                      void *data,
> -                    char *pidfile);
> +                    char *pidfile) ATTRIBUTE_RETURN_CHECK;
>  int virExec(virConnectPtr conn,
>              const char *const*argv,
>              const char *const*envp,
> @@ -78,14 +78,14 @@ int virExec(virConnectPtr conn,
>              int infd,
>              int *outfd,
>              int *errfd,
> -            int flags);
> -int virRun(virConnectPtr conn, const char *const*argv, int *status);
> +            int flags) ATTRIBUTE_RETURN_CHECK;
> +int virRun(virConnectPtr conn, const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK;
>  
> -int virFileReadLimFD(int fd, int maxlen, char **buf);
> +int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK;
>  
> -int virFileReadAll(const char *path, int maxlen, char **buf);
> +int virFileReadAll(const char *path, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK;
>  
> -int virFileWriteStr(const char *path, const char *str);
> +int virFileWriteStr(const char *path, const char *str) ATTRIBUTE_RETURN_CHECK;
>  
>  int virFileMatchesNameSuffix(const char *file,
>                               const char *name,
> @@ -95,28 +95,28 @@ int virFileHasSuffix(const char *str,
>                       const char *suffix);
>  
>  int virFileStripSuffix(char *str,
> -                       const char *suffix);
> +                       const char *suffix) ATTRIBUTE_RETURN_CHECK;
>  
>  int virFileLinkPointsTo(const char *checkLink,
>                          const char *checkDest);
>  
>  int virFileResolveLink(const char *linkpath,
> -                       char **resultpath);
> +                       char **resultpath) ATTRIBUTE_RETURN_CHECK;
>  
>  char *virFindFileInPath(const char *file);
>  
>  int virFileExists(const char *path);
>  
> -int virFileMakePath(const char *path);
> +int virFileMakePath(const char *path) ATTRIBUTE_RETURN_CHECK;
>  
>  int virFileBuildPath(const char *dir,
>                       const char *name,
>                       const char *ext,
>                       char *buf,
> -                     unsigned int buflen);
> +                     unsigned int buflen) ATTRIBUTE_RETURN_CHECK;
>  
>  int virFileAbsPath(const char *path,
> -                   char **abspath);
> +                   char **abspath) ATTRIBUTE_RETURN_CHECK;
>  
>  int virFileOpenTty(int *ttymaster,
>                     char **ttyName,
> @@ -129,13 +129,13 @@ int virFileOpenTtyAt(const char *ptmx,
>  char* virFilePid(const char *dir,
>                   const char *name);
>  int virFileWritePidPath(const char *path,
> -                        pid_t pid);
> +                        pid_t pid) ATTRIBUTE_RETURN_CHECK;
>  int virFileWritePid(const char *dir,
>                      const char *name,
> -                    pid_t pid);
> +                    pid_t pid) ATTRIBUTE_RETURN_CHECK;
>  int virFileReadPid(const char *dir,
>                     const char *name,
> -                   pid_t *pid);
> +                   pid_t *pid) ATTRIBUTE_RETURN_CHECK;
>  int virFileDeletePid(const char *dir,
>                       const char *name);
>  
> @@ -167,7 +167,7 @@ int virMacAddrCompare (const char *mac1, const char *mac2);
>  void virSkipSpaces(const char **str);
>  int virParseNumber(const char **str);
>  int virAsprintf(char **strp, const char *fmt, ...)
> -    ATTRIBUTE_FMT_PRINTF(2, 3);
> +    ATTRIBUTE_FMT_PRINTF(2, 3) ATTRIBUTE_RETURN_CHECK;
>  char *virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)
>      ATTRIBUTE_RETURN_CHECK;
>  char *virStrcpy(char *dest, const char *src, size_t destbytes)
> @@ -179,7 +179,7 @@ char *virStrcpy(char *dest, const char *src, size_t destbytes)
>  #define VIR_MAC_STRING_BUFLEN VIR_MAC_BUFLEN * 3
>  
>  int virParseMacAddr(const char* str,
> -                    unsigned char *addr);
> +                    unsigned char *addr) ATTRIBUTE_RETURN_CHECK;
>  void virFormatMacAddr(const unsigned char *addr,
>                        char *str);
>  void virGenerateMacAddr(const unsigned char *prefix,
> @@ -233,13 +233,13 @@ char *virGetUserName(virConnectPtr conn,
>                       uid_t uid);
>  int virGetUserID(virConnectPtr conn,
>                   const char *name,
> -                 uid_t *uid);
> +                 uid_t *uid) ATTRIBUTE_RETURN_CHECK;
>  int virGetGroupID(virConnectPtr conn,
>                    const char *name,
> -                  gid_t *gid);
> +                  gid_t *gid) ATTRIBUTE_RETURN_CHECK;
>  #endif
>  
> -int virRandomInitialize(unsigned int seed);
> +int virRandomInitialize(unsigned int seed) ATTRIBUTE_RETURN_CHECK;
>  int virRandom(int max);
>  
>  #ifdef HAVE_MNTENT_H
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index 4741c57..aecda23 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -2219,7 +2219,9 @@ static char *vboxDomainDumpXML(virDomainPtr dom, int flags) {
>                                       MACAddress[4], MACAddress[5], MACAddress[6], MACAddress[7],
>                                       MACAddress[8], MACAddress[9], MACAddress[10], MACAddress[11]);
>  
> -                            virParseMacAddr(macaddr, def->nets[netAdpIncCnt]->mac);
> +                            /* XXX some real error handling here some day ... */
> +                            if (virParseMacAddr(macaddr, def->nets[netAdpIncCnt]->mac) < 0)
> +                            {}
>  
>                              netAdpIncCnt++;
>  
> diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c
> index 732b2d3..b52f66e 100644
> --- a/src/xen/xm_internal.c
> +++ b/src/xen/xm_internal.c
> @@ -3035,14 +3035,16 @@ xenXMDomainBlockPeek (virDomainPtr dom,
>  static char *xenXMAutostartLinkName(virDomainPtr dom)
>  {
>      char *ret;
> -    virAsprintf(&ret, "/etc/xen/auto/%s", dom->name);
> +    if (virAsprintf(&ret, "/etc/xen/auto/%s", dom->name) < 0)
> +        return NULL;
>      return ret;
>  }
>  
>  static char *xenXMDomainConfigName(virDomainPtr dom)
>  {
>      char *ret;
> -    virAsprintf(&ret, "/etc/xen/%s", dom->name);
> +    if (virAsprintf(&ret, "/etc/xen/%s", dom->name) < 0)
> +        return NULL;
>      return ret;
>  }
>  
> diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
> index 1b16aa9..5602df0 100644
> --- a/tests/qemuargv2xmltest.c
> +++ b/tests/qemuargv2xmltest.c
> @@ -109,8 +109,6 @@ mymain(int argc, char **argv)
>      if (!abs_srcdir)
>          abs_srcdir = getcwd(cwd, sizeof(cwd));
>  
> -    virRandomInitialize(0);
> -
>      if ((driver.caps = testQemuCapsInit()) == NULL)
>          return EXIT_FAILURE;
>      if((driver.stateDir = strdup("/nowhere")) == NULL)

  ACK ! This takes less time than running the OOM checks :-)

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/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]