[libvirt] [PATCH 05/21] Annotate many methods with ATTRIBUTE_RETURN_CHECK & fix problems
Daniel Veillard
veillard at redhat.com
Wed Oct 28 16:31:36 UTC 2009
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 at veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
More information about the libvir-list
mailing list