On Mon, Feb 02, 2009 at 06:08:14PM +0100, Jim Meyering wrote:

diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 09f69bf..596d940 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -93,31 +93,19 @@ static void qemuDriverUnlock(struct qemud_driver *driver)

static int qemudSetCloseExec(int fd) { int flags; - if ((flags = fcntl(fd, F_GETFD)) < 0) - goto error; - flags |= FD_CLOEXEC; - if ((fcntl(fd, F_SETFD, flags)) < 0) - goto error; - return 0; - error: - qemudLog(QEMUD_ERR, - "%s", _("Failed to set close-on-exec file descriptor flag\n")); - return -1; + return ((flags = fcntl(fd, F_GETFD)) < 0 + || fcntl(fd, F_SETFD, flags | FD_CLOEXEC) < 0 + ? -1 + : 0); }

static int qemudSetNonBlock(int fd) { int flags; - if ((flags = fcntl(fd, F_GETFL)) < 0) - goto error; - flags |= O_NONBLOCK; - if ((fcntl(fd, F_SETFL, flags)) < 0) - goto error; - return 0; - error: - qemudLog(QEMUD_ERR, - "%s", _("Failed to set non-blocking file descriptor flag\n")); - return -1; + return ((flags = fcntl(fd, F_GETFL)) < 0 + || fcntl(fd, F_SETFL, flags | O_NONBLOCK) < 0 + ? -1 + : 0); }

These two chunks can be left out, since those functions are completely removed by the next patch.

@@ -877,8 +865,7 @@ static int qemudWaitForMonitor(virConnectPtr conn, qemudFindCharDevicePTYs, "console", 3000); if (close(logfd) < 0) - qemudLog(QEMUD_WARN, _("Unable to close logfile: %s\n"), - strerror(errno)); + virReportSystemError(NULL, errno, "%s", _("Unable to close logfile"));

This is not fatal to starting the VM, so should raise an error here. Could argue we shoud raise the log level to QEMUD_ERROR though.

@@ -1200,30 +1187,30 @@ static int qemudStartVMDaemon(virConnectPtr conn, tmp = progenv; while (*tmp) { if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0) - qemudLog(QEMUD_WARN, _("Unable to write envv to logfile %d: %s\n"), - errno, strerror(errno)); + virReportSystemError(NULL, errno, + "%s", _("Unable to write envv to logfile")); if (safewrite(vm->logfile, " ", 1) < 0) - qemudLog(QEMUD_WARN, _("Unable to write envv to logfile %d: %s\n"), - errno, strerror(errno)); + virReportSystemError(NULL, errno, + "%s", _("Unable to write envv to logfile")); tmp++; } tmp = argv; while (*tmp) { if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0) - qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"), - errno, strerror(errno)); + virReportSystemError(NULL, errno, + "%s", _("Unable to write argv to logfile")); if (safewrite(vm->logfile, " ", 1) < 0) - qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"), - errno, strerror(errno)); + virReportSystemError(NULL, errno, + "%s", _("Unable to write argv to logfile")); tmp++; } if (safewrite(vm->logfile, "\n", 1) < 0) - qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"), - errno, strerror(errno)); + virReportSystemError(NULL, errno, + "%s", _("Unable to write argv to logfile"));

if ((pos = lseek(vm->logfile, 0, SEEK_END)) < 0) - qemudLog(QEMUD_WARN, _("Unable to seek to end of logfile %d: %s\n"), - errno, strerror(errno)); + virReportSystemError(NULL, errno, + "%s", _("Unable to seek to end of logfile"));

for (i = 0 ; i < ntapfds ; i++) FD_SET(tapfds[i], &keepfd);

Likewise all of those are non-fatal, so shouldn't be raising an error back to the caller, since the overall operation is still reporting success code.

@@ -1300,8 +1288,9 @@ static void qemudShutdownVMDaemon(virConnectPtr conn ATTRIBUTE_UNUSED,

if (virKillProcess(vm->pid, 0) == 0 && virKillProcess(vm->pid, SIGTERM) < 0) - qemudLog(QEMUD_ERROR, _("Failed to send SIGTERM to %s (%d): %s\n"), - vm->def->name, vm->pid, strerror(errno)); + virReportSystemError(conn, errno, + _("Failed to send SIGTERM to %s (%d)"), + vm->def->name, vm->pid);

if (vm->monitor_watch != -1) { virEventRemoveHandle(vm->monitor_watch); @@ -1309,8 +1298,7 @@ static void qemudShutdownVMDaemon(virConnectPtr conn ATTRIBUTE_UNUSED, }

if (close(vm->logfile) < 0) - qemudLog(QEMUD_WARN, _("Unable to close logfile %d: %s\n"), - errno, strerror(errno)); + virReportSystemError(conn, errno, "%s", _("Unable to close logfile")); if (vm->monitor != -1) close(vm->monitor); vm->logfile = -1;

These two are both non-fatal to the proces of shutting down the VM daemon. In facton the 2nd case, I'm wondering why we still have the vm->logfile handle open at all. We are no longer responsible for logging stdout/err from the QEMU proess into the logfile, since we dup'd its stdout/err directly onto the logfile FD. So we shouldnt' have any reason to keep this open.

@@ -1477,8 +1465,8 @@ qemudMonitorCommandExtra(const virDomainObjPtr vm,

/* Log, but ignore failures to write logfile for VM */ if (safewrite(vm->logfile, buf, strlen(buf)) < 0) - qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s\n"), - strerror(errno)); + virReportSystemError(NULL, errno, + "%s", _("Unable to log VM console data"));

*reply = buf; return 0; @@ -1487,8 +1475,8 @@ qemudMonitorCommandExtra(const virDomainObjPtr vm, if (buf) { /* Log, but ignore failures to write logfile for VM */ if (safewrite(vm->logfile, buf, strlen(buf)) < 0) - qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s\n"), - strerror(errno)); + virReportSystemError(NULL, errno, + "%s", _("Unable to log VM console data")); VIR_FREE(buf); } return -1;

These two should stay as logging calls too, since they're not fatal.

@@ -1593,7 +1581,7 @@ static int kvmGetMaxVCPUs(void) {

fd = open(KVM_DEVICE, O_RDONLY); if (fd < 0) { - qemudLog(QEMUD_WARN, _("Unable to open %s: %s\n"), KVM_DEVICE, strerror(errno)); + virReportSystemError(NULL, errno, _("Unable to open %s"), KVM_DEVICE); return maxvcpus; }

This needs to report a real error code back to the user, since if we are using KVM vms, then /dev/kvm should always exist. 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" <berrange@redhat.com> wrote: ...

...

@@ -877,8 +865,7 @@ static int qemudWaitForMonitor(virConnectPtr conn, qemudFindCharDevicePTYs, "console", 3000); if (close(logfd) < 0) - qemudLog(QEMUD_WARN, _("Unable to close logfile: %s\n"), - strerror(errno)); + virReportSystemError(NULL, errno, "%s", _("Unable to close logfile"));

This is not fatal to starting the VM, so should raise an error here. Could argue we shoud raise the log level to QEMUD_ERROR though.

FYI, whether we use QEMUD_ERROR or QEMUD_WARN doesn't really matter, since that parameter is ignored by the qemudLog macro: #define qemudLog(level, msg...) fprintf(stderr, msg)

"Daniel P. Berrange" <berrange@redhat.com> wrote: ...

...

- qemudLog(QEMUD_ERR, - "%s", _("Failed to set non-blocking file descriptor flag\n")); - return -1; + return ((flags = fcntl(fd, F_GETFL)) < 0 + || fcntl(fd, F_SETFL, flags | O_NONBLOCK) < 0 + ? -1 + : 0); }

These two chunks can be left out, since those functions are completely removed by the next patch.

...

@@ -877,8 +865,7 @@ static int qemudWaitForMonitor(virConnectPtr conn, qemudFindCharDevicePTYs, "console", 3000); if (close(logfd) < 0) - qemudLog(QEMUD_WARN, _("Unable to close logfile: %s\n"), - strerror(errno)); + virReportSystemError(NULL, errno, "%s", _("Unable to close logfile"));

This is not fatal to starting the VM, so should raise an error here. Could argue we shoud raise the log level to QEMUD_ERROR though.

Worst still, there are two symbols used here, QEMUD_ERR QEMUD_ERROR neither of which is defined. That's technically fine, since they're both preprocessed away, but makes searching for occurrences (like I was just doing) prone to error if you happen to use a word-restricted search or search only for the longer string. I'll normalize to QEMUD_ERROR while I'm here.

On Mon, Feb 02, 2009 at 06:08:15PM +0100, Jim Meyering wrote:

From: Jim Meyering <meyering@redhat.com>

* src/qemu_driver.c: Use virSetCloseExec and virSetNonBlock, rather than qemuSet* functions. Suggested by Daniel P. Berrange. * src/util.c (virSetCloseExec): Compile unconditionally. * src/util.h (virSetCloseExec): Declare

diff --git a/qemud/Makefile.am b/qemud/Makefile.am index a0c161a..372b931 100644 --- a/qemud/Makefile.am +++ b/qemud/Makefile.am @@ -107,6 +107,7 @@ libvirtd_LDADD = \ if ! WITH_DRIVER_MODULES if WITH_QEMU libvirtd_LDADD += ../src/libvirt_driver_qemu.la +libvirtd_LDADD += ../src/libvirt_util.la endif

This is not required. All the functions in util.c are provided in the libvirt.so, and exported with LIBVIRT_PRIVATE_X_X_X tag. This patch is missing the change to libvirt_private.sym to actually add the virSetCloseExec function though.

@@ -182,10 +182,7 @@ int virSetNonBlock(int fd) { return 0; }

- -#ifndef WIN32 - -static int virSetCloseExec(int fd) { +int virSetCloseExec(int fd) { int flags; if ((flags = fcntl(fd, F_GETFD)) < 0) return -1; @@ -195,6 +192,8 @@ static int virSetCloseExec(int fd) { return 0; }

+#ifndef WIN32 + static int __virExec(virConnectPtr conn, const char *const*argv,

Why this change ? AFAIK, there is no close-on-exec flag in Win32, since it doesn't have any concept of exec(). The QEMU driver isn't built on Win32, so we shouldn't need to expose virSetCloseExec there anyway. 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" <berrange@redhat.com> wrote:

On Mon, Feb 02, 2009 at 06:08:17PM +0100, Jim Meyering wrote:

...

From: Jim Meyering <meyering@redhat.com> * src/iptables.c: Include "virterror_internal.h". Use virStrerror, not strerror. * src/virterror.c (virStrerror): Remove "static". * src/virterror_internal.h (virStrerror): Declare it. ... -static const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) +const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) ... +const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen);

This seems to be missing ta change to libvirt_private.sym to actually export the new function to individual drivers.

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:

"Daniel P. Berrange" <berrange@redhat.com> wrote:

On Wed, Feb 04, 2009 at 01:07:49PM +0100, Jim Meyering wrote: ...

...

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 ... 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

Someting seems not right here - we shouldn't need this change, since this shouldn't be added in the first place.

That's the *incremental* change, against what you reviewed the first time.

...

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)); }

This also appears to be changing code which doesn't exist.

Same as above.

[snip more of the same]

Have you got the actual patch against current CVS, since this doesn't appear to be it ?

Yep. It was attached at the end, after this:

...

Here's the 10-change-set sequence:

"Daniel P. Berrange" <berrange@redhat.com> wrote:

On Thu, Feb 05, 2009 at 02:30:02PM +0100, Jim Meyering wrote:

...

"Daniel P. Berrange" <berrange@redhat.com> wrote:

...

On Wed, Feb 04, 2009 at 01:07:49PM +0100, Jim Meyering wrote: ...

...

Here's an incremental diff that also includes additional changes, e.g., ^^^^^^^^^^^^^^^^

...

...

[snip more of the same]

Have you got the actual patch against current CVS, since this doesn't appear to be it ?

Yep. It was attached at the end, after this:

...

...

Here's the 10-change-set sequence:

This is the section I was quoting - it a seems to be identical to the incremental diff.

Oh. So it is. Sorry about that. Here's the full diff:

From 51de11e2ec42e65b756d714afe94b02cd27e38e9 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 3 Feb 2009 21:42:39 +0100 Subject: [PATCH 01/10] publicize virStrerror

* src/virterror.c (virStrerror): Remove "static". * src/virterror_internal.h (virStrerror): Declare it. * src/libvirt_private.syms: Add virStrerror; --- src/libvirt_private.syms | 1 + src/virterror.c | 2 +- src/virterror_internal.h | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a6f4f3e..17aefbe 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -326,6 +326,7 @@ virErrorMsg; virRaiseError; virReportSystemErrorFull; virReportOOMErrorFull; +virStrerror; # xml.h diff --git a/src/virterror.c b/src/virterror.c index 160fea3..6c479f4 100644 --- a/src/virterror.c +++ b/src/virterror.c @@ -1005,7 +1005,7 @@ void virReportErrorHelper(virConnectPtr conn, int domcode, int errcode, } -static const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) +const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) { #ifdef HAVE_STRERROR_R # ifdef __USE_GNU diff --git a/src/virterror_internal.h b/src/virterror_internal.h index 3a11705..709c8ec 100644 --- a/src/virterror_internal.h +++ b/src/virterror_internal.h @@ -1,7 +1,7 @@ /* * virterror.h: internal error handling * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2009 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -81,5 +81,6 @@ void virReportOOMErrorFull(virConnectPtr conn, void virSetGlobalError(void); void virSetConnError(virConnectPtr conn); +const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen); #endif -- 1.6.1.2.511.gc5d3f

From a5cf1250b325ae2d1fa62b45bcdfff47c97da035 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 28 Jan 2009 19:20:08 +0100 Subject: [PATCH 02/10] qemu_driver.c: use virReportSystemError in place of some qemudLog uses

...thus eliminating many uses of strerror. (kvmGetMaxVCPUs): Rather than merely warning, diagnose an error and return -1 upon failure to open KVM_DEVICE. Suggested by Daniel P. Berrange. --- src/qemu_driver.c | 97 +++++++++++++++++++++++++++-------------------------- 1 files changed, 49 insertions(+), 48 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index ebcdd88..3a790c1 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -207,22 +207,21 @@ qemudLogReadFD(virConnectPtr conn, const char* logDir, const char* name, off_t p if ((fd = open(logfile, logmode)) < 0) { - qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("failed to create logfile %s: %s"), - logfile, strerror(errno)); + virReportSystemError(conn, errno, + _("failed to create logfile %s"), + logfile); return -1; } if (qemudSetCloseExec(fd) < 0) { - qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Unable to set VM logfile close-on-exec flag: %s"), - strerror(errno)); + virReportSystemError(conn, errno, "%s", + _("Unable to set VM logfile close-on-exec flag")); close(fd); return -1; } if (lseek(fd, pos, SEEK_SET) < 0) { - qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Unable to seek to %lld in %s: %s"), - (long long) pos, logfile, strerror(errno)); + virReportSystemError(conn, errno, + _("Unable to seek to %lld in %s"), + (long long) pos, logfile); close(fd); } return fd; @@ -250,7 +249,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 { @@ -336,7 +335,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; } @@ -346,13 +345,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; } @@ -451,7 +450,7 @@ qemudStartup(void) { } if (virFileMakePath(qemu_driver->stateDir) < 0) { - qemudLog(QEMUD_ERR, _("Failed to create state dir '%s': %s\n"), + qemudLog(QEMUD_ERROR, _("Failed to create state dir '%s': %s\n"), qemu_driver->stateDir, strerror(errno)); goto error; } @@ -493,7 +492,7 @@ qemudStartup(void) { return 0; out_of_memory: - qemudLog (QEMUD_ERR, + qemudLog (QEMUD_ERROR, "%s", _("qemudStartup: out of memory\n")); error: if (qemu_driver) @@ -1200,30 +1199,30 @@ static int qemudStartVMDaemon(virConnectPtr conn, tmp = progenv; while (*tmp) { if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0) - qemudLog(QEMUD_WARN, _("Unable to write envv to logfile %d: %s\n"), - errno, strerror(errno)); + qemudLog(QEMUD_WARN, _("Unable to write envv to logfile: %s\n"), + strerror(errno)); if (safewrite(vm->logfile, " ", 1) < 0) - qemudLog(QEMUD_WARN, _("Unable to write envv to logfile %d: %s\n"), - errno, strerror(errno)); + qemudLog(QEMUD_WARN, _("Unable to write envv to logfile: %s\n"), + strerror(errno)); tmp++; } tmp = argv; while (*tmp) { if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0) - qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"), - errno, strerror(errno)); + qemudLog(QEMUD_WARN, _("Unable to write argv to logfile: %s\n"), + strerror(errno)); if (safewrite(vm->logfile, " ", 1) < 0) - qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"), - errno, strerror(errno)); + qemudLog(QEMUD_WARN, _("Unable to write argv to logfile: %s\n"), + strerror(errno)); tmp++; } if (safewrite(vm->logfile, "\n", 1) < 0) - qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"), - errno, strerror(errno)); + qemudLog(QEMUD_WARN, _("Unable to write argv to logfile: %s\n"), + strerror(errno)); if ((pos = lseek(vm->logfile, 0, SEEK_END)) < 0) - qemudLog(QEMUD_WARN, _("Unable to seek to end of logfile %d: %s\n"), - errno, strerror(errno)); + qemudLog(QEMUD_WARN, _("Unable to seek to end of logfile: %s\n"), + strerror(errno)); for (i = 0 ; i < ntapfds ; i++) FD_SET(tapfds[i], &keepfd); @@ -1292,7 +1291,8 @@ static int qemudStartVMDaemon(virConnectPtr conn, static void qemudShutdownVMDaemon(virConnectPtr conn ATTRIBUTE_UNUSED, - struct qemud_driver *driver, virDomainObjPtr vm) { + struct qemud_driver *driver, + virDomainObjPtr vm) { if (!virDomainIsActive(vm)) return; @@ -1300,8 +1300,9 @@ static void qemudShutdownVMDaemon(virConnectPtr conn ATTRIBUTE_UNUSED, if (virKillProcess(vm->pid, 0) == 0 && virKillProcess(vm->pid, SIGTERM) < 0) - qemudLog(QEMUD_ERROR, _("Failed to send SIGTERM to %s (%d): %s\n"), - vm->def->name, vm->pid, strerror(errno)); + virReportSystemError(conn, errno, + _("Failed to send SIGTERM to %s (%d)"), + vm->def->name, vm->pid); if (vm->monitor_watch != -1) { virEventRemoveHandle(vm->monitor_watch); @@ -1309,8 +1310,8 @@ static void qemudShutdownVMDaemon(virConnectPtr conn ATTRIBUTE_UNUSED, } if (close(vm->logfile) < 0) - qemudLog(QEMUD_WARN, _("Unable to close logfile %d: %s\n"), - errno, strerror(errno)); + qemudLog(QEMUD_WARN, _("Unable to close logfile: %s\n"), + strerror(errno)); if (vm->monitor != -1) close(vm->monitor); vm->logfile = -1; @@ -1593,8 +1594,8 @@ static int kvmGetMaxVCPUs(void) { fd = open(KVM_DEVICE, O_RDONLY); if (fd < 0) { - qemudLog(QEMUD_WARN, _("Unable to open %s: %s\n"), KVM_DEVICE, strerror(errno)); - return maxvcpus; + virReportSystemError(NULL, errno, _("Unable to open %s"), KVM_DEVICE); + return -1; } r = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); @@ -1836,8 +1837,8 @@ qemudGetHostname (virConnectPtr conn) result = virGetHostname(); if (result == NULL) { - qemudReportError (conn, NULL, NULL, VIR_ERR_SYSTEM_ERROR, - "%s", strerror (errno)); + virReportSystemError (conn, errno, + "%s", _("failed to determine host name")); return NULL; } /* Caller frees this string. */ @@ -3922,8 +3923,8 @@ qemudDomainBlockPeek (virDomainPtr dom, /* The path is correct, now try to open it and get its size. */ fd = open (path, O_RDONLY); if (fd == -1) { - qemudReportError (dom->conn, dom, NULL, VIR_ERR_SYSTEM_ERROR, - "%s", strerror (errno)); + virReportSystemError (dom->conn, errno, + _("%s: failed to open"), path); goto cleanup; } @@ -3933,8 +3934,8 @@ qemudDomainBlockPeek (virDomainPtr dom, */ if (lseek (fd, offset, SEEK_SET) == (off_t) -1 || saferead (fd, buffer, size) == (ssize_t) -1) { - qemudReportError (dom->conn, dom, NULL, VIR_ERR_SYSTEM_ERROR, - "%s", strerror (errno)); + virReportSystemError (dom->conn, errno, + _("%s: failed to seek or read"), path); goto cleanup; } @@ -3988,8 +3989,8 @@ qemudDomainMemoryPeek (virDomainPtr dom, /* Create a temporary filename. */ if ((fd = mkstemp (tmp)) == -1) { - qemudReportError (dom->conn, dom, NULL, VIR_ERR_SYSTEM_ERROR, - "%s", strerror (errno)); + virReportSystemError (dom->conn, errno, + _("mkstemp(\"%s\") failed"), tmp); goto cleanup; } @@ -4005,8 +4006,9 @@ qemudDomainMemoryPeek (virDomainPtr dom, /* Read the memory file into buffer. */ if (saferead (fd, buffer, size) == (ssize_t) -1) { - qemudReportError (dom->conn, dom, NULL, VIR_ERR_SYSTEM_ERROR, - "%s", strerror (errno)); + virReportSystemError (dom->conn, errno, + _("failed to read temporary file " + "created with template %s"), tmp); goto cleanup; } @@ -4165,8 +4167,8 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, /* Get hostname */ if (gethostname (hostname, HOST_NAME_MAX+1) == -1) { - qemudReportError (dconn, NULL, NULL, VIR_ERR_SYSTEM_ERROR, - "%s", strerror (errno)); + virReportSystemError (dconn, errno, + "%s", _("failed to determine host name")); goto cleanup; } @@ -4336,8 +4338,7 @@ qemudDomainMigratePerform (virDomainPtr dom, /* Issue the migrate command. */ safe_uri = qemudEscapeMonitorArg (uri); if (!safe_uri) { - qemudReportError (dom->conn, dom, NULL, VIR_ERR_SYSTEM_ERROR, - "%s", strerror (errno)); + virReportOOMError (dom->conn); goto cleanup; } snprintf (cmd, sizeof cmd, "migrate \"%s\"", safe_uri); -- 1.6.1.2.511.gc5d3f

From dc64e55aa9f1dc7700d83bf767b436b2980f10f4 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 30 Jan 2009 21:37:20 +0100 Subject: [PATCH 03/10] remove duplicate *SetCloseExec and *SetNonBlock functions

* src/qemu_driver.c: Use virSetCloseExec and virSetNonBlock, rather than qemuSet* functions. Suggested by Daniel P. Berrange. * src/util.c (virSetCloseExec): Publicize. * src/util.h (virSetCloseExec): Declare * src/libvirt_private.syms: Add virSetCloseExec. --- src/libvirt_private.syms | 1 + src/qemu_driver.c | 39 ++++----------------------------------- src/util.c | 4 ++-- src/util.h | 1 + 4 files changed, 8 insertions(+), 37 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 17aefbe..4338da7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -292,6 +292,7 @@ virEnumToString; virEventAddHandle; virEventRemoveHandle; virExec; +virSetCloseExec; virSetNonBlock; virFormatMacAddr; virGetHostname; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 3a790c1..8d8d711 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -91,37 +91,6 @@ static void qemuDriverUnlock(struct qemud_driver *driver) virMutexUnlock(&driver->lock); } -static int qemudSetCloseExec(int fd) { - int flags; - if ((flags = fcntl(fd, F_GETFD)) < 0) - goto error; - flags |= FD_CLOEXEC; - if ((fcntl(fd, F_SETFD, flags)) < 0) - goto error; - return 0; - error: - qemudLog(QEMUD_ERR, - "%s", _("Failed to set close-on-exec file descriptor flag\n")); - return -1; -} - - -static int qemudSetNonBlock(int fd) { - int flags; - if ((flags = fcntl(fd, F_GETFL)) < 0) - goto error; - flags |= O_NONBLOCK; - if ((fcntl(fd, F_SETFL, flags)) < 0) - goto error; - return 0; - error: - qemudLog(QEMUD_ERR, - "%s", _("Failed to set non-blocking file descriptor flag\n")); - return -1; -} - - - static void qemuDomainEventFlush(int timer, void *opaque); static void qemuDomainEventQueue(struct qemud_driver *driver, virDomainEventPtr event); @@ -180,7 +149,7 @@ qemudLogFD(virConnectPtr conn, const char* logDir, const char* name) logfile); return -1; } - if (qemudSetCloseExec(fd) < 0) { + if (virSetCloseExec(fd) < 0) { virReportSystemError(conn, errno, "%s", _("Unable to set VM logfile close-on-exec flag")); close(fd); @@ -212,7 +181,7 @@ qemudLogReadFD(virConnectPtr conn, const char* logDir, const char* name, off_t p logfile); return -1; } - if (qemudSetCloseExec(fd) < 0) { + if (virSetCloseExec(fd) < 0) { virReportSystemError(conn, errno, "%s", _("Unable to set VM logfile close-on-exec flag")); close(fd); @@ -721,12 +690,12 @@ static int qemudOpenMonitor(virConnectPtr conn, _("Unable to open monitor path %s"), monitor); return -1; } - if (qemudSetCloseExec(monfd) < 0) { + if (virSetCloseExec(monfd) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to set monitor close-on-exec flag")); goto error; } - if (qemudSetNonBlock(monfd) < 0) { + if (virSetNonBlock(monfd) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to put monitor into non-blocking mode")); goto error; diff --git a/src/util.c b/src/util.c index 7c7fe31..96c1b00 100644 --- a/src/util.c +++ b/src/util.c @@ -1,7 +1,7 @@ /* * utils.c: common, generic utility functions * - * Copyright (C) 2006, 2007, 2008 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain @@ -185,7 +185,7 @@ int virSetNonBlock(int fd) { #ifndef WIN32 -static int virSetCloseExec(int fd) { +int virSetCloseExec(int fd) { int flags; if ((flags = fcntl(fd, F_GETFD)) < 0) return -1; diff --git a/src/util.h b/src/util.h index c553264..4667b92 100644 --- a/src/util.h +++ b/src/util.h @@ -39,6 +39,7 @@ enum { }; int virSetNonBlock(int fd); +int virSetCloseExec(int fd); int virExec(virConnectPtr conn, const char *const*argv, -- 1.6.1.2.511.gc5d3f

From 929ea43e5c36d3e7b44d3855ad8956ba7f5a5d0f Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 3 Feb 2009 21:51:42 +0100 Subject: [PATCH 04/10] qemu_driver.c: s/strerror/virStrerror.../

Perform most changs automatically, with this: perl -pi -e 's/\bstrerror *\((.+?)\)/virStrerror($1, ebuf, sizeof ebuf)/' Then, declare ebuf, as needed --- src/qemu_driver.c | 40 +++++++++++++++++++++++++--------------- 1 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 8d8d711..da25063 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -419,8 +419,9 @@ qemudStartup(void) { } if (virFileMakePath(qemu_driver->stateDir) < 0) { + char ebuf[1024]; qemudLog(QEMUD_ERROR, _("Failed to create state dir '%s': %s\n"), - qemu_driver->stateDir, strerror(errno)); + qemu_driver->stateDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } @@ -844,9 +845,11 @@ static int qemudWaitForMonitor(virConnectPtr conn, ret = qemudReadMonitorOutput(conn, vm, logfd, buf, sizeof(buf), qemudFindCharDevicePTYs, "console", 3000); - if (close(logfd) < 0) + if (close(logfd) < 0) { + char ebuf[1024]; qemudLog(QEMUD_WARN, _("Unable to close logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); + } if (ret == 1) /* Success */ return 0; @@ -1098,6 +1101,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, const char *emulator; pid_t child; int pos = -1; + char ebuf[1024]; FD_ZERO(&keepfd); @@ -1169,29 +1173,29 @@ static int qemudStartVMDaemon(virConnectPtr conn, while (*tmp) { if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0) qemudLog(QEMUD_WARN, _("Unable to write envv to logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); if (safewrite(vm->logfile, " ", 1) < 0) qemudLog(QEMUD_WARN, _("Unable to write envv to logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); tmp++; } tmp = argv; while (*tmp) { if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0) qemudLog(QEMUD_WARN, _("Unable to write argv to logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); if (safewrite(vm->logfile, " ", 1) < 0) qemudLog(QEMUD_WARN, _("Unable to write argv to logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); tmp++; } if (safewrite(vm->logfile, "\n", 1) < 0) qemudLog(QEMUD_WARN, _("Unable to write argv to logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); if ((pos = lseek(vm->logfile, 0, SEEK_END)) < 0) qemudLog(QEMUD_WARN, _("Unable to seek to end of logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); for (i = 0 ; i < ntapfds ; i++) FD_SET(tapfds[i], &keepfd); @@ -1278,9 +1282,11 @@ static void qemudShutdownVMDaemon(virConnectPtr conn ATTRIBUTE_UNUSED, vm->monitor_watch = -1; } - if (close(vm->logfile) < 0) + if (close(vm->logfile) < 0) { + char ebuf[1024]; qemudLog(QEMUD_WARN, _("Unable to close logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); + } if (vm->monitor != -1) close(vm->monitor); vm->logfile = -1; @@ -1446,9 +1452,11 @@ qemudMonitorCommandExtra(const virDomainObjPtr vm, } /* Log, but ignore failures to write logfile for VM */ - if (safewrite(vm->logfile, buf, strlen(buf)) < 0) + if (safewrite(vm->logfile, buf, strlen(buf)) < 0) { + char ebuf[1024]; qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); + } *reply = buf; return 0; @@ -1456,9 +1464,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) + if (safewrite(vm->logfile, buf, strlen(buf)) < 0) { + char ebuf[1024]; qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); + } VIR_FREE(buf); } return -1; -- 1.6.1.2.511.gc5d3f

From 9fc4162e009f0bfaae1096103c8f980aff4bfbc0 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 30 Jan 2009 23:18:04 +0100 Subject: [PATCH 05/10] use virReportOOMError rather than more verbose equivalent

* src/qemu_driver.c (qemudDomainSave, qemudDomainRestore) (qemudDomainAttachPciDiskDevice) (qemudDomainAttachUsbMassstorageDevice): --- src/qemu_driver.c | 18 ++++++------------ 1 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index da25063..a322486 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -462,8 +462,7 @@ qemudStartup(void) { return 0; out_of_memory: - qemudLog (QEMUD_ERROR, - "%s", _("qemudStartup: out of memory\n")); + virReportOOMError(NULL); error: if (qemu_driver) qemuDriverUnlock(qemu_driver); @@ -2419,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; } @@ -2741,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; } @@ -3220,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; } @@ -3290,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; } -- 1.6.1.2.511.gc5d3f

From eac7054c8e9919c7c4940328902ed5acfae7a260 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 2 Feb 2009 10:03:35 +0100 Subject: [PATCH 06/10] iptables.c: Use virStrerror, not strerror.

* src/iptables.c: Include "virterror_internal.h". Use virStrerror, not strerror. * src/iptables.c (notifyRulesUpdated): Use %s rather than string-concatenation that made sc_unmarked_diagnostics report a false-positive. --- src/iptables.c | 23 +++++++++++++++-------- 1 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/iptables.c b/src/iptables.c index c850b9e..9011277 100644 --- a/src/iptables.c +++ b/src/iptables.c @@ -44,6 +44,7 @@ #include "iptables.h" #include "util.h" #include "memory.h" +#include "virterror_internal.h" #define qemudLog(level, msg...) fprintf(stderr, msg) @@ -98,9 +99,10 @@ notifyRulesUpdated(const char *table, argv[2] = arg; argv[3] = NULL; + char ebuf[1024]; if (virRun(NULL, argv, NULL) < 0) - qemudLog(QEMUD_WARN, _("Failed to run '" LOKKIT_PATH - " %s' : %s"), arg, strerror(errno)); + qemudLog(QEMUD_WARN, _("Failed to run '%s %s': %s"), + LOKKIT_PATH, arg, virStrerror(errno, ebuf, sizeof ebuf)); } static int @@ -174,10 +176,11 @@ notifyRulesRemoved(const char *table, return; - write_error: + write_error:; + char ebuf[1024]; qemudLog(QEMUD_WARN, _("Failed to write to " SYSCONF_DIR "/sysconfig/system-config-firewall : %s"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); if (f) fclose(f); VIR_FREE(content); @@ -239,15 +242,16 @@ iptRulesSave(iptRules *rules) #ifdef ENABLE_IPTABLES_LOKKIT int err; + char ebuf[1024]; if ((err = virFileMakePath(rules->dir))) { qemudLog(QEMUD_WARN, _("Failed to create directory %s : %s"), - rules->dir, strerror(err)); + rules->dir, virStrerror(err, ebuf, sizeof ebuf)); return; } if ((err = writeRules(rules->path, rules->rules, rules->nrules))) { qemudLog(QEMUD_WARN, _("Failed to saves iptables rules to %s : %s"), - rules->path, strerror(err)); + rules->path, virStrerror(err, ebuf, sizeof ebuf)); return; } @@ -537,6 +541,7 @@ static void iptRulesReload(iptRules *rules) { int i; + char ebuf[1024]; for (i = 0; i < rules->nrules; i++) { iptRule *rule = &rules->rules[i]; @@ -549,7 +554,8 @@ iptRulesReload(iptRules *rules) qemudLog(QEMUD_WARN, _("Failed to remove iptables rule '%s'" " from chain '%s' in table '%s': %s"), - rule->rule, rules->chain, rules->table, strerror(errno)); + rule->rule, rules->chain, rules->table, + virStrerror(errno, ebuf, sizeof ebuf)); rule->argv[rule->command_idx] = orig; } @@ -558,7 +564,8 @@ iptRulesReload(iptRules *rules) if (virRun(NULL, rules->rules[i].argv, NULL) < 0) qemudLog(QEMUD_WARN, _("Failed to add iptables rule '%s'" " to chain '%s' in table '%s': %s"), - rules->rules[i].rule, rules->chain, rules->table, strerror(errno)); + rules->rules[i].rule, rules->chain, rules->table, + virStrerror(errno, ebuf, sizeof ebuf)); } /** -- 1.6.1.2.511.gc5d3f

From 04e6663f82a19a138109eab08d85ef56b246780a Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 2 Feb 2009 11:26:33 +0100 Subject: [PATCH 07/10] qemud.c: use virStrerror, not strerror

--- qemud/qemud.c | 65 +++++++++++++++++++++++++++++++++++++------------------- 1 files changed, 43 insertions(+), 22 deletions(-) diff --git a/qemud/qemud.c b/qemud/qemud.c index fa5e17d..48083df 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -231,8 +231,9 @@ remoteCheckCertFile(const char *type, const char *file) { struct stat sb; if (stat(file, &sb) < 0) { - VIR_ERROR(_("Cannot access %s '%s': %s (%d)"), - type, file, strerror(errno), errno); + char ebuf[1024]; + VIR_ERROR(_("Cannot access %s '%s': %s"), + type, file, virStrerror(errno, ebuf, sizeof ebuf)); return -1; } return 0; @@ -331,7 +332,9 @@ qemudDispatchSignalEvent(int watch ATTRIBUTE_UNUSED, virMutexLock(&server->lock); if (saferead(server->sigread, &siginfo, sizeof(siginfo)) != sizeof(siginfo)) { - VIR_ERROR(_("Failed to read from signal pipe: %s"), strerror(errno)); + char ebuf[1024]; + VIR_ERROR(_("Failed to read from signal pipe: %s"), + virStrerror(errno, ebuf, sizeof ebuf)); virMutexUnlock(&server->lock); return; } @@ -459,33 +462,34 @@ static int qemudGoDaemon(void) { static int qemudWritePidFile(const char *pidFile) { int fd; FILE *fh; + char ebuf[1024]; if (pidFile[0] == '\0') return 0; if ((fd = open(pidFile, O_WRONLY|O_CREAT|O_EXCL, 0644)) < 0) { VIR_ERROR(_("Failed to open pid file '%s' : %s"), - pidFile, strerror(errno)); + pidFile, virStrerror(errno, ebuf, sizeof ebuf)); return -1; } if (!(fh = fdopen(fd, "w"))) { VIR_ERROR(_("Failed to fdopen pid file '%s' : %s"), - pidFile, strerror(errno)); + pidFile, virStrerror(errno, ebuf, sizeof ebuf)); close(fd); return -1; } if (fprintf(fh, "%lu\n", (unsigned long)getpid()) < 0) { VIR_ERROR(_("Failed to write to pid file '%s' : %s"), - pidFile, strerror(errno)); + pidFile, virStrerror(errno, ebuf, sizeof ebuf)); close(fd); return -1; } if (fclose(fh) == EOF) { VIR_ERROR(_("Failed to close pid file '%s' : %s"), - pidFile, strerror(errno)); + pidFile, virStrerror(errno, ebuf, sizeof ebuf)); return -1; } @@ -498,6 +502,7 @@ static int qemudListenUnix(struct qemud_server *server, struct sockaddr_un addr; mode_t oldmask; gid_t oldgrp; + char ebuf[1024]; if (VIR_ALLOC(sock) < 0) { VIR_ERROR("%s", _("Failed to allocate memory for struct qemud_socket")); @@ -511,7 +516,7 @@ static int qemudListenUnix(struct qemud_server *server, if ((sock->fd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { VIR_ERROR(_("Failed to create socket: %s"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); goto cleanup; } @@ -533,7 +538,7 @@ static int qemudListenUnix(struct qemud_server *server, if (bind(sock->fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) { VIR_ERROR(_("Failed to bind socket to '%s': %s"), - path, strerror(errno)); + path, virStrerror(errno, ebuf, sizeof ebuf)); goto cleanup; } umask(oldmask); @@ -542,7 +547,7 @@ static int qemudListenUnix(struct qemud_server *server, if (listen(sock->fd, 30) < 0) { VIR_ERROR(_("Failed to listen for connections on '%s': %s"), - path, strerror(errno)); + path, virStrerror(errno, ebuf, sizeof ebuf)); goto cleanup; } @@ -587,10 +592,11 @@ remoteMakeSockets (int *fds, int max_fds, int *nfds_r, const char *node, const c struct addrinfo *runp = ai; while (runp && *nfds_r < max_fds) { + char ebuf[1024]; fds[*nfds_r] = socket (runp->ai_family, runp->ai_socktype, runp->ai_protocol); if (fds[*nfds_r] == -1) { - VIR_ERROR(_("socket: %s"), strerror (errno)); + VIR_ERROR(_("socket: %s"), virStrerror (errno, ebuf, sizeof ebuf)); return -1; } @@ -599,14 +605,15 @@ remoteMakeSockets (int *fds, int max_fds, int *nfds_r, const char *node, const c if (bind (fds[*nfds_r], runp->ai_addr, runp->ai_addrlen) == -1) { if (errno != EADDRINUSE) { - VIR_ERROR(_("bind: %s"), strerror (errno)); + VIR_ERROR(_("bind: %s"), virStrerror (errno, ebuf, sizeof ebuf)); return -1; } close (fds[*nfds_r]); } else { if (listen (fds[*nfds_r], SOMAXCONN) == -1) { - VIR_ERROR(_("listen: %s"), strerror (errno)); + VIR_ERROR(_("listen: %s"), + virStrerror (errno, ebuf, sizeof ebuf)); return -1; } ++*nfds_r; @@ -638,10 +645,12 @@ remoteListenTCP (struct qemud_server *server, for (i = 0; i < nfds; ++i) { struct sockaddr_storage sa; + char ebuf[1024]; socklen_t salen = sizeof(sa); if (VIR_ALLOC(sock) < 0) { - VIR_ERROR(_("remoteListenTCP: calloc: %s"), strerror (errno)); + VIR_ERROR(_("remoteListenTCP: calloc: %s"), + virStrerror (errno, ebuf, sizeof ebuf)); goto cleanup; } @@ -671,7 +680,8 @@ remoteListenTCP (struct qemud_server *server, goto cleanup; if (listen (sock->fd, 30) < 0) { - VIR_ERROR(_("remoteListenTCP: listen: %s"), strerror (errno)); + VIR_ERROR(_("remoteListenTCP: listen: %s"), + virStrerror (errno, ebuf, sizeof ebuf)); goto cleanup; } @@ -1145,8 +1155,9 @@ int qemudGetSocketIdentity(int fd, uid_t *uid, pid_t *pid) { unsigned int cr_len = sizeof (cr); if (getsockopt (fd, SOL_SOCKET, SO_PEERCRED, &cr, &cr_len) < 0) { + char ebuf[1024]; VIR_ERROR(_("Failed to verify client credentials: %s"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); return -1; } @@ -1169,9 +1180,11 @@ static int qemudDispatchServer(struct qemud_server *server, struct qemud_socket int no_slow_start = 1; if ((fd = accept(sock->fd, (struct sockaddr *)&addr, &addrlen)) < 0) { + char ebuf[1024]; if (errno == EAGAIN) return 0; - VIR_ERROR(_("Failed to accept connection: %s"), strerror(errno)); + VIR_ERROR(_("Failed to accept connection: %s"), + virStrerror(errno, ebuf, sizeof ebuf)); return -1; } @@ -1476,13 +1489,15 @@ static ssize_t qemudClientReadBuf(struct qemud_client *client, /*qemudDebug ("qemudClientRead: len = %d", len);*/ if (!client->tlssession) { + char ebuf[1024]; ret = read (client->fd, data, len); if (ret == -1 && (errno == EAGAIN || errno == EINTR)) return 0; if (ret <= 0) { if (ret != 0) - VIR_ERROR(_("read: %s"), strerror (errno)); + VIR_ERROR(_("read: %s"), + virStrerror (errno, ebuf, sizeof ebuf)); qemudDispatchClientFailure(client); return -1; } @@ -1700,10 +1715,11 @@ static ssize_t qemudClientWriteBuf(struct qemud_client *client, } if (!client->tlssession) { + char ebuf[1024]; if ((ret = write(client->fd, data, len)) == -1) { if (errno == EAGAIN || errno == EINTR) return 0; - VIR_ERROR(_("write: %s"), strerror (errno)); + VIR_ERROR(_("write: %s"), virStrerror (errno, ebuf, sizeof ebuf)); qemudDispatchClientFailure(client); return -1; } @@ -2004,9 +2020,10 @@ static int qemudOneLoop(void) { /* Check for any signal handling errors and log them. */ errors = sig_errors; if (errors) { + char ebuf[1024]; sig_errors -= errors; VIR_ERROR(_("Signal handler reported %d errors: last error: %s"), - errors, strerror (sig_lasterrno)); + errors, virStrerror (sig_lasterrno, ebuf, sizeof ebuf)); return -1; } @@ -2743,8 +2760,10 @@ int main(int argc, char **argv) { } if (godaemon) { + char ebuf[1024]; if (qemudGoDaemon() < 0) { - VIR_ERROR(_("Failed to fork as daemon: %s"), strerror(errno)); + VIR_ERROR(_("Failed to fork as daemon: %s"), + virStrerror(errno, ebuf, sizeof ebuf)); goto error1; } } @@ -2765,7 +2784,9 @@ int main(int argc, char **argv) { qemudSetNonBlock(sigpipe[1]) < 0 || qemudSetCloseExec(sigpipe[0]) < 0 || qemudSetCloseExec(sigpipe[1]) < 0) { - VIR_ERROR(_("Failed to create pipe: %s"), strerror(errno)); + char ebuf[1024]; + VIR_ERROR(_("Failed to create pipe: %s"), + virStrerror(errno, ebuf, sizeof ebuf)); goto error2; } sigwrite = sigpipe[1]; -- 1.6.1.2.511.gc5d3f

From e5d1e5781fb9f7b4e04bbb51908897b0d732148d Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 2 Feb 2009 12:20:04 +0100 Subject: [PATCH 08/10] don't include raw errno in diagnostics

* src/uml_driver.c (umlStartVMDaemon): Don't print raw errno value. * qemud/remote.c (remoteDispatchAuthSaslInit): Likewise. --- qemud/remote.c | 10 +++++----- src/uml_driver.c | 22 +++++++++++----------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/qemud/remote.c b/qemud/remote.c index d064d61..2e0c48e 100644 --- a/qemud/remote.c +++ b/qemud/remote.c @@ -1,7 +1,7 @@ /* * remote.c: code handling remote requests (from remote_internal.c) * - * Copyright (C) 2007, 2008 Red Hat, Inc. + * Copyright (C) 2007, 2008, 2009 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -2572,8 +2572,8 @@ remoteDispatchAuthSaslInit (struct qemud_server *server, salen = sizeof(sa); if (getsockname(client->fd, (struct sockaddr*)&sa, &salen) < 0) { remoteDispatchFormatError(rerr, - _("failed to get sock address %d (%s)"), - errno, strerror(errno)); + _("failed to get sock address: %s"), + strerror(errno)); goto error; } if ((localAddr = addrToString(rerr, &sa, salen)) == NULL) { @@ -2583,8 +2583,8 @@ remoteDispatchAuthSaslInit (struct qemud_server *server, /* Get remote address in form IPADDR:PORT */ salen = sizeof(sa); if (getpeername(client->fd, (struct sockaddr*)&sa, &salen) < 0) { - remoteDispatchFormatError(rerr, _("failed to get peer address %d (%s)"), - errno, strerror(errno)); + remoteDispatchFormatError(rerr, _("failed to get peer address: %s"), + strerror(errno)); VIR_FREE(localAddr); goto error; } diff --git a/src/uml_driver.c b/src/uml_driver.c index 1d12406..0fe2fac 100644 --- a/src/uml_driver.c +++ b/src/uml_driver.c @@ -1,7 +1,7 @@ /* * uml_driver.c: core driver methods for managing UML guests * - * Copyright (C) 2006, 2007, 2008 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -791,26 +791,26 @@ static int umlStartVMDaemon(virConnectPtr conn, tmp = progenv; while (*tmp) { if (safewrite(logfd, *tmp, strlen(*tmp)) < 0) - umlLog(VIR_LOG_WARN, _("Unable to write envv to logfile %d: %s\n"), - errno, strerror(errno)); + umlLog(VIR_LOG_WARN, _("Unable to write envv to logfile: %s\n"), + strerror(errno)); if (safewrite(logfd, " ", 1) < 0) - umlLog(VIR_LOG_WARN, _("Unable to write envv to logfile %d: %s\n"), - errno, strerror(errno)); + umlLog(VIR_LOG_WARN, _("Unable to write envv to logfile: %s\n"), + strerror(errno)); tmp++; } tmp = argv; while (*tmp) { if (safewrite(logfd, *tmp, strlen(*tmp)) < 0) - umlLog(VIR_LOG_WARN, _("Unable to write argv to logfile %d: %s\n"), - errno, strerror(errno)); + umlLog(VIR_LOG_WARN, _("Unable to write argv to logfile: %s\n"), + strerror(errno)); if (safewrite(logfd, " ", 1) < 0) - umlLog(VIR_LOG_WARN, _("Unable to write argv to logfile %d: %s\n"), - errno, strerror(errno)); + umlLog(VIR_LOG_WARN, _("Unable to write argv to logfile: %s\n"), + strerror(errno)); tmp++; } if (safewrite(logfd, "\n", 1) < 0) - umlLog(VIR_LOG_WARN, _("Unable to write argv to logfile %d: %s\n"), - errno, strerror(errno)); + umlLog(VIR_LOG_WARN, _("Unable to write argv to logfile: %s\n"), + strerror(errno)); vm->monitor = -1; -- 1.6.1.2.511.gc5d3f

From ca8d2f5fb341a5624603fb6b99adb6412f66b8a6 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 2 Feb 2009 13:34:02 +0100 Subject: [PATCH 09/10] remove remainder of offending strerror uses

* qemud/qemud.c (GET_CONF_STR): Use virStrerror, not strerror. * qemud/remote.c (remoteDispatchDomainBlockPeek): Likewise. (remoteDispatchDomainMemoryPeek, remoteDispatchAuthSaslInit): Likewise. (remoteDispatchAuthPolkit): Likewise. * src/lxc_container.c (lxcContainerAvailable): Likewise. * src/network_driver.c (networkStartNetworkDaemon): Likewise. (networkShutdownNetworkDaemon): Likewise. * src/qemu_conf.c (qemudExtractVersion, qemudNetworkIfaceConnect): * src/storage_conf.c (virStoragePoolLoadAllConfigs): Likewise. * src/storage_driver.c (storagePoolUndefine): Likewise. * src/uml_driver.c (umlStartup, umlStartVMDaemon): Likewise. * src/util.c (virFileReadAll): Likewise. * src/uuid.c (virUUIDGenerate): Likewise. * src/xen_internal.c (get_cpu_flags): Likewise. --- qemud/qemud.c | 5 ++++- qemud/remote.c | 26 ++++++++++++++++++-------- src/lxc_container.c | 4 ++-- src/network_driver.c | 11 +++++++---- src/qemu_conf.c | 8 +++++--- src/storage_conf.c | 5 +++-- src/storage_driver.c | 8 +++++--- src/uml_driver.c | 16 +++++++++------- src/util.c | 6 ++++-- src/uuid.c | 10 +++++++--- src/xen_internal.c | 3 ++- 11 files changed, 66 insertions(+), 36 deletions(-) diff --git a/qemud/qemud.c b/qemud/qemud.c index 48083df..effb336 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -49,6 +49,7 @@ #include <netdb.h> #include "libvirt_internal.h" +#include "virterror_internal.h" #include "qemud.h" #include "util.h" @@ -2316,7 +2317,9 @@ checkType (virConfValuePtr p, const char *filename, goto free_and_fail; \ (var_name) = strdup (p->str); \ if ((var_name) == NULL) { \ - VIR_ERROR(_("remoteReadConfigFile: %s\n"),strerror (errno)); \ + char ebuf[1024]; \ + VIR_ERROR(_("remoteReadConfigFile: %s\n"), \ + virStrerror(errno, ebuf, sizeof ebuf)); \ goto free_and_fail; \ } \ } \ diff --git a/qemud/remote.c b/qemud/remote.c index 2e0c48e..78dda42 100644 --- a/qemud/remote.c +++ b/qemud/remote.c @@ -41,6 +41,7 @@ #include <string.h> #include <errno.h> #include <fnmatch.h> +#include "virterror_internal.h" #ifdef HAVE_POLKIT #include <polkit/polkit.h> @@ -990,9 +991,11 @@ remoteDispatchDomainBlockPeek (struct qemud_server *server ATTRIBUTE_UNUSED, } ret->buffer.buffer_len = size; - if (VIR_ALLOC_N(ret->buffer.buffer_val, size) < 0) { + if (VIR_ALLOC_N (ret->buffer.buffer_val, size) < 0) { + char ebuf[1024]; virDomainFree (dom); - remoteDispatchFormatError (rerr, "%s", strerror (errno)); + remoteDispatchFormatError (rerr, "%s", + virStrerror(errno, ebuf, sizeof ebuf)); return -1; } @@ -1031,16 +1034,18 @@ remoteDispatchDomainMemoryPeek (struct qemud_server *server ATTRIBUTE_UNUSED, flags = args->flags; if (size > REMOTE_DOMAIN_MEMORY_PEEK_BUFFER_MAX) { + virDomainFree (dom); remoteDispatchFormatError (rerr, "%s", _("size > maximum buffer size")); - virDomainFree (dom); return -1; } ret->buffer.buffer_len = size; if (VIR_ALLOC_N (ret->buffer.buffer_val, size) < 0) { - remoteDispatchFormatError (rerr, "%s", strerror (errno)); + char ebuf[1024]; virDomainFree (dom); + remoteDispatchFormatError (rerr, "%s", + virStrerror(errno, ebuf, sizeof ebuf)); return -1; } @@ -2571,9 +2576,10 @@ remoteDispatchAuthSaslInit (struct qemud_server *server, /* Get local address in form IPADDR:PORT */ salen = sizeof(sa); if (getsockname(client->fd, (struct sockaddr*)&sa, &salen) < 0) { + char ebuf[1024]; remoteDispatchFormatError(rerr, _("failed to get sock address: %s"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); goto error; } if ((localAddr = addrToString(rerr, &sa, salen)) == NULL) { @@ -2583,8 +2589,9 @@ remoteDispatchAuthSaslInit (struct qemud_server *server, /* Get remote address in form IPADDR:PORT */ salen = sizeof(sa); if (getpeername(client->fd, (struct sockaddr*)&sa, &salen) < 0) { + char ebuf[1024]; remoteDispatchFormatError(rerr, _("failed to get peer address: %s"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); VIR_FREE(localAddr); goto error; } @@ -3062,7 +3069,9 @@ remoteDispatchAuthPolkit (struct qemud_server *server, } if (!(pkaction = polkit_action_new())) { - VIR_ERROR(_("Failed to create polkit action %s\n"), strerror(errno)); + char ebuf[1024]; + VIR_ERROR(_("Failed to create polkit action %s\n"), + virStrerror(errno, ebuf, sizeof ebuf)); polkit_caller_unref(pkcaller); goto authfail; } @@ -3070,9 +3079,10 @@ remoteDispatchAuthPolkit (struct qemud_server *server, if (!(pkcontext = polkit_context_new()) || !polkit_context_init(pkcontext, &pkerr)) { + char ebuf[1024]; VIR_ERROR(_("Failed to create polkit context %s\n"), (pkerr ? polkit_error_get_error_message(pkerr) - : strerror(errno))); + : virStrerror(errno, ebuf, sizeof ebuf))); if (pkerr) polkit_error_free(pkerr); polkit_caller_unref(pkcaller); diff --git a/src/lxc_container.c b/src/lxc_container.c index ea52eed..f1c523b 100644 --- a/src/lxc_container.c +++ b/src/lxc_container.c @@ -668,8 +668,9 @@ int lxcContainerAvailable(int features) cpid = clone(lxcContainerDummyChild, childStack, flags, NULL); VIR_FREE(stack); if (cpid < 0) { + char ebuf[1024]; DEBUG("clone call returned %s, container support is not enabled", - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); return -1; } else { waitpid(cpid, &childStatus, 0); @@ -677,4 +678,3 @@ int lxcContainerAvailable(int features) return 0; } - diff --git a/src/network_driver.c b/src/network_driver.c index dcc6ad9..4138939 100644 --- a/src/network_driver.c +++ b/src/network_driver.c @@ -907,14 +907,16 @@ static int networkStartNetworkDaemon(virConnectPtr conn, 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, strerror(err)); + 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, strerror(err)); + network->def->bridge, virStrerror(err, ebuf, sizeof ebuf)); } return -1; @@ -944,15 +946,16 @@ static int networkShutdownNetworkDaemon(virConnectPtr conn, networkRemoveIptablesRules(driver, network); + char ebuf[1024]; if (network->def->ipAddress && (err = brSetInterfaceUp(driver->brctl, network->def->bridge, 0))) { networkLog(NETWORK_WARN, _("Failed to bring down bridge '%s' : %s\n"), - network->def->bridge, strerror(err)); + network->def->bridge, virStrerror(err, ebuf, sizeof ebuf)); } if ((err = brDeleteBridge(driver->brctl, network->def->bridge))) { networkLog(NETWORK_WARN, _("Failed to delete bridge '%s' : %s\n"), - network->def->bridge, strerror(err)); + network->def->bridge, virStrerror(err, ebuf, sizeof ebuf)); } /* See if its still alive and really really kill it */ diff --git a/src/qemu_conf.c b/src/qemu_conf.c index ef45b12..6f58ee8 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -517,9 +517,10 @@ int qemudExtractVersion(virConnectPtr conn, return -1; if (stat(binary, &sb) < 0) { + char ebuf[1024]; qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Cannot find QEMU binary %s: %s"), binary, - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); return -1; } @@ -580,10 +581,11 @@ qemudNetworkIfaceConnect(virConnectPtr conn, } } + char ebuf[1024]; if (!driver->brctl && (err = brInit(&driver->brctl))) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("cannot initialize bridge support: %s"), - strerror(err)); + virStrerror(err, ebuf, sizeof ebuf)); goto error; } @@ -598,7 +600,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Failed to add tap interface '%s' " "to bridge '%s' : %s"), - net->ifname, brname, strerror(err)); + net->ifname, brname, virStrerror(err, ebuf, sizeof ebuf)); } goto error; } diff --git a/src/storage_conf.c b/src/storage_conf.c index 7eb89e7..70107a2 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -1,7 +1,7 @@ /* * storage_conf.c: config handling for storage driver * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2009 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -1384,10 +1384,11 @@ virStoragePoolLoadAllConfigs(virConnectPtr conn, struct dirent *entry; if (!(dir = opendir(configDir))) { + char ebuf[1024]; if (errno == ENOENT) return 0; virStorageLog("Failed to open dir '%s': %s", - configDir, strerror(errno)); + configDir, virStrerror(errno, ebuf, sizeof ebuf)); return -1; } diff --git a/src/storage_driver.c b/src/storage_driver.c index a456061..f1320c5 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -1,7 +1,7 @@ /* * storage_driver.c: core driver for storage APIs * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2009 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -567,9 +567,11 @@ storagePoolUndefine(virStoragePoolPtr obj) { if (virStoragePoolObjDeleteDef(obj->conn, pool) < 0) goto cleanup; - 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, strerror(errno)); + 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 0fe2fac..c5a06a2 100644 --- a/src/uml_driver.c +++ b/src/uml_driver.c @@ -376,8 +376,9 @@ umlStartup(void) { } if (virFileMakePath(uml_driver->monitorDir) < 0) { + char ebuf[1024]; umlLog(VIR_LOG_ERROR, _("Failed to create monitor directory %s: %s"), - uml_driver->monitorDir, strerror(errno)); + uml_driver->monitorDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } @@ -726,6 +727,7 @@ static int umlStartVMDaemon(virConnectPtr conn, int *tapfds = NULL; int ntapfds = 0; fd_set keepfd; + char ebuf[1024]; FD_ZERO(&keepfd); @@ -792,25 +794,25 @@ static int umlStartVMDaemon(virConnectPtr conn, while (*tmp) { if (safewrite(logfd, *tmp, strlen(*tmp)) < 0) umlLog(VIR_LOG_WARN, _("Unable to write envv to logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); if (safewrite(logfd, " ", 1) < 0) umlLog(VIR_LOG_WARN, _("Unable to write envv to logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); tmp++; } tmp = argv; while (*tmp) { if (safewrite(logfd, *tmp, strlen(*tmp)) < 0) umlLog(VIR_LOG_WARN, _("Unable to write argv to logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); if (safewrite(logfd, " ", 1) < 0) umlLog(VIR_LOG_WARN, _("Unable to write argv to logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); tmp++; } if (safewrite(logfd, "\n", 1) < 0) umlLog(VIR_LOG_WARN, _("Unable to write argv to logfile: %s\n"), - strerror(errno)); + virStrerror(errno, ebuf, sizeof ebuf)); vm->monitor = -1; @@ -825,7 +827,7 @@ static int umlStartVMDaemon(virConnectPtr conn, /* Cleanup intermediate proces */ if (waitpid(pid, NULL, 0) != pid) umlLog(VIR_LOG_WARN, _("failed to wait on process: %d: %s\n"), - pid, strerror(errno)); + pid, virStrerror(errno, ebuf, sizeof ebuf)); for (i = 0 ; argv[i] ; i++) VIR_FREE(argv[i]); diff --git a/src/util.c b/src/util.c index 96c1b00..01fe37a 100644 --- a/src/util.c +++ b/src/util.c @@ -755,17 +755,19 @@ int virFileReadLimFD(int fd_arg, int maxlen, char **buf) int virFileReadAll(const char *path, int maxlen, char **buf) { + char ebuf[1024]; FILE *fh = fopen(path, "r"); if (fh == NULL) { virLog("Failed to open file '%s': %s\n", - path, strerror(errno)); + path, virStrerror(errno, ebuf, sizeof ebuf)); return -1; } int len = virFileReadLimFP (fh, maxlen, buf); fclose(fh); if (len < 0) { - virLog("Failed to read '%s': %s\n", path, strerror (errno)); + virLog("Failed to read '%s': %s\n", path, + virStrerror(errno, ebuf, sizeof ebuf)); return -1; } diff --git a/src/uuid.c b/src/uuid.c index 9d263de..6f7d85f 100644 --- a/src/uuid.c +++ b/src/uuid.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007, 2008 Red Hat, Inc. + * Copyright (C) 2007, 2008, 2009 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -36,6 +36,7 @@ #include "c-ctype.h" #include "internal.h" #include "util.h" +#include "virterror_internal.h" #define qemudLog(level, msg...) fprintf(stderr, msg) @@ -99,10 +100,13 @@ virUUIDGenerate(unsigned char *uuid) if (uuid == NULL) return(-1); - 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"), strerror(err)); + " failed to generate random bytes: %s"), + virStrerror(err, ebuf, sizeof ebuf)); + } return virUUIDGeneratePseudoRandomBytes(uuid, VIR_UUID_BUFLEN); } diff --git a/src/xen_internal.c b/src/xen_internal.c index 0a01f5e..a866af1 100644 --- a/src/xen_internal.c +++ b/src/xen_internal.c @@ -2265,8 +2265,9 @@ get_cpu_flags(virConnectPtr conn, const char **hvm, int *pae, int *longmode) if ((fd = open("/dev/cpu/self/cpuid", O_RDONLY)) == -1 || pread(fd, ®s, sizeof(regs), 0) != sizeof(regs)) { + char ebuf[1024]; virXenError(conn, VIR_ERR_SYSTEM_ERROR, - "couldn't read CPU flags: %s", strerror(errno)); + "couldn't read CPU flags: %s", virStrerror(errno, ebuf, sizeof ebuf)); goto out; } -- 1.6.1.2.511.gc5d3f

From 214b4d2b01559f85198f16fa34a877407f457c44 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 2 Feb 2009 13:47:05 +0100 Subject: [PATCH 10/10] syntax-check: enable prohibit_nonreentrant

* .x-sc_prohibit_nonreentrant: Add src/console.c and virterror.c. * Makefile.cfg (local-checks-to-skip): Don't exempt sc_prohibit_nonreentrant. * Makefile.nonreentrant: Don't exempt strerror. --- .x-sc_prohibit_nonreentrant | 4 +++- Makefile.nonreentrant | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.x-sc_prohibit_nonreentrant b/.x-sc_prohibit_nonreentrant index a90e010..cc323b0 100644 --- a/.x-sc_prohibit_nonreentrant +++ b/.x-sc_prohibit_nonreentrant @@ -4,5 +4,7 @@ ChangeLog ^Makefile* ^docs/ ^tests/ -^src/virsh.c +^src/virsh\.c +^src/console\.c +^src/virterror\.c ^build-aux/ 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 -- 1.6.1.2.511.gc5d3f

On Mon, Feb 02, 2009 at 06:08:20PM +0100, Jim Meyering wrote:

From: Jim Meyering <meyering@redhat.com>

* qemud/qemud.c (GET_CONF_STR): Use virStrerror, not strerror. * qemud/remote.c (remoteDispatchDomainBlockPeek): Likewise. (remoteDispatchDomainMemoryPeek, remoteDispatchAuthSaslInit): Likewise. (remoteDispatchAuthPolkit): Likewise. * src/lxc_container.c (lxcContainerAvailable): Likewise. * src/network_driver.c (networkStartNetworkDaemon): Likewise. (networkShutdownNetworkDaemon): Likewise. * src/qemu_conf.c (qemudExtractVersion, qemudNetworkIfaceConnect): * src/storage_conf.c (virStoragePoolLoadAllConfigs): Likewise. * src/storage_driver.c (storagePoolUndefine): Likewise. * src/uml_driver.c (umlStartup, umlStartVMDaemon): Likewise. * src/util.c (virFileReadAll): Likewise. * src/uuid.c (virUUIDGenerate): Likewise. * src/xen_internal.c (get_cpu_flags): Likewise.

diff --git a/src/network_driver.c b/src/network_driver.c index dcc6ad9..8d7340e 100644 --- a/src/network_driver.c +++ b/src/network_driver.c @@ -904,17 +904,18 @@ static int networkStartNetworkDaemon(virConnectPtr conn, err_delbr2: networkRemoveIptablesRules(driver, network);

- err_delbr1: + err_delbr1:; + char ebuf[1024];

A stray ';' crept in there.

diff --git a/src/storage_driver.c b/src/storage_driver.c index a456061..d644b63 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -1,7 +1,7 @@ /* * storage_driver.c: core driver for storage APIs * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2009 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -567,9 +567,10 @@ storagePoolUndefine(virStoragePoolPtr obj) { if (virStoragePoolObjDeleteDef(obj->conn, pool) < 0) goto cleanup;

+ char ebuf[1024]; if (unlink(pool->autostartLink) < 0 && errno != ENOENT && errno != ENOTDIR) storageLog("Failed to delete autostart link '%s': %s", - pool->autostartLink, strerror(errno)); + pool->autostartLink, virStrerror(errno, ebuf, sizeof ebuf));

The variable decl should be inside the if () {} clause there.

@@ -788,29 +789,30 @@ static int umlStartVMDaemon(virConnectPtr conn, return -1; }

+ char ebuf[1024]; tmp = progenv; while (*tmp) {

Here, the var decl should be inside the while() loop too

@@ -99,10 +100,12 @@ virUUIDGenerate(unsigned char *uuid) if (uuid == NULL) return(-1);

+ char ebuf[1024]; if ((err = virUUIDGenerateRandomBytes(uuid, VIR_UUID_BUFLEN))) qemudLog(QEMUD_WARN, _("Falling back to pseudorandom UUID," - " failed to generate random bytes: %s"), strerror(err)); + " failed to generate random bytes: %s"), + virStrerror(err, ebuf, sizeof ebuf));

return virUUIDGeneratePseudoRandomBytes(uuid, VIR_UUID_BUFLEN);

AGain the ebuf needs to move inside the block 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 :|

On Tue, Feb 03, 2009 at 04:06:18PM +0100, Jim Meyering wrote:

...

...

- err_delbr1: + err_delbr1:; + char ebuf[1024];

A stray ';' crept in there.

Actually, it's required by C. The ";" adds an empty statement before the new declaration, to serve as a target for the label.

That's pretty horrible :-( I'd prefer it to just move the ebuf inside the following if() {} block where its used, or to the other var decls at the start of the function.

...

...

@@ -788,29 +789,30 @@ static int umlStartVMDaemon(virConnectPtr conn, return -1; }

+ char ebuf[1024]; tmp = progenv; while (*tmp) {

Here, the var decl should be inside the while() loop too

Well, if I move the decl into that first while loop, then I'd have to add 3 more, one into the next while loop, and then two more, each into currently-single-stmt if-blocks. Seems slightly better to add just that one decl than to add 4.

If its used in lots of places in the function, then its clearer to declare at the start of the function. 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 :|