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

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) -- 1.6.2.5