Re: [libvirt] [PATCH 4/9] iptables.c: Use virStrerror, not strerror.

"Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: ... ... Good catch. I didn't notice because currently it isn't needed. I've fixed that and addressed all of your other comments. Here's an incremental diff that also includes additional changes, e.g., - enable the syntax-check for strerror - change more "out of error" reports to use virReportOOMError - use QEMUD_ERROR consistently After that, I've included the full, revised sequence of changes. Also, not apparent in this incremental, I moved some changes between change sets and split some, so now there are 10 change sets. I confirmed that things build after each, just in case via this: b=034-remove-strerror git checkout $b git checkout -b tmp $b for rev in $(git rev-list HEAD ^master|tac); do git reset --hard $rev git log -1 make syntax-check > makerr.sc 2>&1 || break make check > makerr.c 2>&1 || break done diff --git a/Makefile.nonreentrant b/Makefile.nonreentrant index 13fa59d..b567f31 100644 --- a/Makefile.nonreentrant +++ b/Makefile.nonreentrant @@ -79,7 +79,7 @@ NON_REENTRANT += setstate NON_REENTRANT += sgetspent NON_REENTRANT += srand48 NON_REENTRANT += srandom -# NON_REENTRANT += strerror +NON_REENTRANT += strerror NON_REENTRANT += strtok NON_REENTRANT += tmpnam NON_REENTRANT += ttyname diff --git a/qemud/Makefile.am b/qemud/Makefile.am index 372b931..a0c161a 100644 --- a/qemud/Makefile.am +++ b/qemud/Makefile.am @@ -107,7 +107,6 @@ libvirtd_LDADD = \ if ! WITH_DRIVER_MODULES if WITH_QEMU libvirtd_LDADD += ../src/libvirt_driver_qemu.la -libvirtd_LDADD += ../src/libvirt_util.la endif if WITH_LXC diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a6f4f3e..4338da7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -292,6 +292,7 @@ virEnumToString; virEventAddHandle; virEventRemoveHandle; virExec; +virSetCloseExec; virSetNonBlock; virFormatMacAddr; virGetHostname; @@ -326,6 +327,7 @@ virErrorMsg; virRaiseError; virReportSystemErrorFull; virReportOOMErrorFull; +virStrerror; # xml.h diff --git a/src/network_driver.c b/src/network_driver.c index 8d7340e..4138939 100644 --- a/src/network_driver.c +++ b/src/network_driver.c @@ -904,16 +904,17 @@ static int networkStartNetworkDaemon(virConnectPtr conn, err_delbr2: networkRemoveIptablesRules(driver, network); - err_delbr1:; - char ebuf[1024]; + err_delbr1: if (network->def->ipAddress && (err = brSetInterfaceUp(driver->brctl, network->def->bridge, 0))) { + char ebuf[1024]; networkLog(NETWORK_WARN, _("Failed to bring down bridge '%s' : %s\n"), network->def->bridge, virStrerror(err, ebuf, sizeof ebuf)); } err_delbr: if ((err = brDeleteBridge(driver->brctl, network->def->bridge))) { + char ebuf[1024]; networkLog(NETWORK_WARN, _("Failed to delete bridge '%s' : %s\n"), network->def->bridge, virStrerror(err, ebuf, sizeof ebuf)); } diff --git a/src/qemu_driver.c b/src/qemu_driver.c index c331087..a322486 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -218,7 +218,7 @@ qemudAutostartConfigs(struct qemud_driver *driver) { int ret = qemudStartVMDaemon(conn, driver, vm, NULL, -1); if (ret < 0) { virErrorPtr err = virGetLastError(); - qemudLog(QEMUD_ERR, _("Failed to autostart VM '%s': %s\n"), + qemudLog(QEMUD_ERROR, _("Failed to autostart VM '%s': %s\n"), vm->def->name, err ? err->message : NULL); } else { @@ -304,7 +304,7 @@ qemudReconnectVMs(struct qemud_driver *driver) if ((config = virDomainConfigFile(NULL, driver->stateDir, vm->def->name)) == NULL) { - qemudLog(QEMUD_ERR, _("Failed to read domain status for %s\n"), + qemudLog(QEMUD_ERROR, _("Failed to read domain status for %s\n"), vm->def->name); goto next_error; } @@ -314,13 +314,13 @@ qemudReconnectVMs(struct qemud_driver *driver) vm->newDef = vm->def; vm->def = status->def; } else { - qemudLog(QEMUD_ERR, _("Failed to parse domain status for %s\n"), + qemudLog(QEMUD_ERROR, _("Failed to parse domain status for %s\n"), vm->def->name); goto next_error; } if ((rc = qemudOpenMonitor(NULL, driver, vm, status->monitorpath, 1)) != 0) { - qemudLog(QEMUD_ERR, _("Failed to reconnect monitor for %s: %d\n"), + qemudLog(QEMUD_ERROR, _("Failed to reconnect monitor for %s: %d\n"), vm->def->name, rc); goto next_error; } @@ -419,9 +419,9 @@ qemudStartup(void) { } if (virFileMakePath(qemu_driver->stateDir) < 0) { - virReportSystemError(NULL, errno, - _("Failed to create state dir '%s'"), - qemu_driver->stateDir); + char ebuf[1024]; + qemudLog(QEMUD_ERROR, _("Failed to create state dir '%s': %s\n"), + qemu_driver->stateDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } @@ -844,8 +844,11 @@ static int qemudWaitForMonitor(virConnectPtr conn, ret = qemudReadMonitorOutput(conn, vm, logfd, buf, sizeof(buf), qemudFindCharDevicePTYs, "console", 3000); - if (close(logfd) < 0) - virReportSystemError(NULL, errno, "%s", _("Unable to close logfile")); + if (close(logfd) < 0) { + char ebuf[1024]; + qemudLog(QEMUD_WARN, _("Unable to close logfile: %s\n"), + virStrerror(errno, ebuf, sizeof ebuf)); + } if (ret == 1) /* Success */ return 0; @@ -1097,6 +1100,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, const char *emulator; pid_t child; int pos = -1; + char ebuf[1024]; FD_ZERO(&keepfd); @@ -1167,30 +1171,30 @@ static int qemudStartVMDaemon(virConnectPtr conn, tmp = progenv; while (*tmp) { if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0) - virReportSystemError(NULL, errno, - "%s", _("Unable to write envv to logfile")); + qemudLog(QEMUD_WARN, _("Unable to write envv to logfile: %s\n"), + virStrerror(errno, ebuf, sizeof ebuf)); if (safewrite(vm->logfile, " ", 1) < 0) - virReportSystemError(NULL, errno, - "%s", _("Unable to write envv to logfile")); + qemudLog(QEMUD_WARN, _("Unable to write envv to logfile: %s\n"), + virStrerror(errno, ebuf, sizeof ebuf)); tmp++; } tmp = argv; while (*tmp) { if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0) - virReportSystemError(NULL, errno, - "%s", _("Unable to write argv to logfile")); + qemudLog(QEMUD_WARN, _("Unable to write argv to logfile: %s\n"), + virStrerror(errno, ebuf, sizeof ebuf)); if (safewrite(vm->logfile, " ", 1) < 0) - virReportSystemError(NULL, errno, - "%s", _("Unable to write argv to logfile")); + qemudLog(QEMUD_WARN, _("Unable to write argv to logfile: %s\n"), + virStrerror(errno, ebuf, sizeof ebuf)); tmp++; } if (safewrite(vm->logfile, "\n", 1) < 0) - virReportSystemError(NULL, errno, - "%s", _("Unable to write argv to logfile")); + qemudLog(QEMUD_WARN, _("Unable to write argv to logfile: %s\n"), + virStrerror(errno, ebuf, sizeof ebuf)); if ((pos = lseek(vm->logfile, 0, SEEK_END)) < 0) - virReportSystemError(NULL, errno, - "%s", _("Unable to seek to end of logfile")); + qemudLog(QEMUD_WARN, _("Unable to seek to end of logfile: %s\n"), + virStrerror(errno, ebuf, sizeof ebuf)); for (i = 0 ; i < ntapfds ; i++) FD_SET(tapfds[i], &keepfd); @@ -1277,8 +1281,11 @@ static void qemudShutdownVMDaemon(virConnectPtr conn ATTRIBUTE_UNUSED, vm->monitor_watch = -1; } - if (close(vm->logfile) < 0) - virReportSystemError(conn, errno, "%s", _("Unable to close logfile")); + if (close(vm->logfile) < 0) { + char ebuf[1024]; + qemudLog(QEMUD_WARN, _("Unable to close logfile: %s\n"), + virStrerror(errno, ebuf, sizeof ebuf)); + } if (vm->monitor != -1) close(vm->monitor); vm->logfile = -1; @@ -1444,9 +1451,11 @@ qemudMonitorCommandExtra(const virDomainObjPtr vm, } /* Log, but ignore failures to write logfile for VM */ - if (safewrite(vm->logfile, buf, strlen(buf)) < 0) - virReportSystemError(NULL, errno, - "%s", _("Unable to log VM console data")); + if (safewrite(vm->logfile, buf, strlen(buf)) < 0) { + char ebuf[1024]; + qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s\n"), + virStrerror(errno, ebuf, sizeof ebuf)); + } *reply = buf; return 0; @@ -1454,9 +1463,11 @@ qemudMonitorCommandExtra(const virDomainObjPtr vm, error: if (buf) { /* Log, but ignore failures to write logfile for VM */ - if (safewrite(vm->logfile, buf, strlen(buf)) < 0) - virReportSystemError(NULL, errno, - "%s", _("Unable to log VM console data")); + if (safewrite(vm->logfile, buf, strlen(buf)) < 0) { + char ebuf[1024]; + qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s\n"), + virStrerror(errno, ebuf, sizeof ebuf)); + } VIR_FREE(buf); } return -1; @@ -1562,7 +1573,7 @@ static int kvmGetMaxVCPUs(void) { fd = open(KVM_DEVICE, O_RDONLY); if (fd < 0) { virReportSystemError(NULL, errno, _("Unable to open %s"), KVM_DEVICE); - return maxvcpus; + return -1; } r = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); @@ -2407,15 +2418,13 @@ static int qemudDomainSave(virDomainPtr dom, /* Migrate to file */ safe_path = qemudEscapeShellArg(path); if (!safe_path) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - "%s", _("out of memory")); + virReportOOMError(dom->conn); goto cleanup; } if (virAsprintf(&command, "migrate \"exec:" "dd of='%s' oflag=append conv=notrunc 2>/dev/null" "\"", safe_path) == -1) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - "%s", _("out of memory")); + virReportOOMError(dom->conn); command = NULL; goto cleanup; } @@ -2729,8 +2738,7 @@ static int qemudDomainRestore(virConnectPtr conn, } if (VIR_ALLOC_N(xml, header.xml_len) < 0) { - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - "%s", _("out of memory")); + virReportOOMError(conn); goto cleanup; } @@ -3208,8 +3216,7 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, safe_path = qemudEscapeMonitorArg(dev->data.disk->src); if (!safe_path) { - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - "%s", _("out of memory")); + virReportOOMError(conn); return -1; } @@ -3278,8 +3285,7 @@ static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, safe_path = qemudEscapeMonitorArg(dev->data.disk->src); if (!safe_path) { - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - "%s", _("out of memory")); + virReportOOMError(conn); return -1; } diff --git a/src/storage_driver.c b/src/storage_driver.c index d644b63..f1320c5 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -567,10 +567,11 @@ storagePoolUndefine(virStoragePoolPtr obj) { if (virStoragePoolObjDeleteDef(obj->conn, pool) < 0) goto cleanup; - char ebuf[1024]; - if (unlink(pool->autostartLink) < 0 && errno != ENOENT && errno != ENOTDIR) + if (unlink(pool->autostartLink) < 0 && errno != ENOENT && errno != ENOTDIR) { + char ebuf[1024]; storageLog("Failed to delete autostart link '%s': %s", pool->autostartLink, virStrerror(errno, ebuf, sizeof ebuf)); + } VIR_FREE(pool->configFile); VIR_FREE(pool->autostartLink); diff --git a/src/uml_driver.c b/src/uml_driver.c index 48e2d07..c5a06a2 100644 --- a/src/uml_driver.c +++ b/src/uml_driver.c @@ -727,6 +727,7 @@ static int umlStartVMDaemon(virConnectPtr conn, int *tapfds = NULL; int ntapfds = 0; fd_set keepfd; + char ebuf[1024]; FD_ZERO(&keepfd); @@ -789,7 +790,6 @@ static int umlStartVMDaemon(virConnectPtr conn, return -1; } - char ebuf[1024]; tmp = progenv; while (*tmp) { if (safewrite(logfd, *tmp, strlen(*tmp)) < 0) diff --git a/src/util.c b/src/util.c index 1034455..01fe37a 100644 --- a/src/util.c +++ b/src/util.c @@ -182,6 +182,9 @@ int virSetNonBlock(int fd) { return 0; } + +#ifndef WIN32 + int virSetCloseExec(int fd) { int flags; if ((flags = fcntl(fd, F_GETFD)) < 0) @@ -192,8 +195,6 @@ int virSetCloseExec(int fd) { return 0; } -#ifndef WIN32 - static int __virExec(virConnectPtr conn, const char *const*argv, diff --git a/src/uuid.c b/src/uuid.c index 9e9416f..6f7d85f 100644 --- a/src/uuid.c +++ b/src/uuid.c @@ -100,12 +100,13 @@ virUUIDGenerate(unsigned char *uuid) if (uuid == NULL) return(-1); - char ebuf[1024]; - if ((err = virUUIDGenerateRandomBytes(uuid, VIR_UUID_BUFLEN))) + if ((err = virUUIDGenerateRandomBytes(uuid, VIR_UUID_BUFLEN))) { + char ebuf[1024]; qemudLog(QEMUD_WARN, _("Falling back to pseudorandom UUID," " failed to generate random bytes: %s"), virStrerror(err, ebuf, sizeof ebuf)); + } return virUUIDGeneratePseudoRandomBytes(uuid, VIR_UUID_BUFLEN); } Here's the 10-change-set sequence: