On Thu, Jan 29, 2009 at 09:54:59PM +0100, Jim Meyering wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 36e12b2..8fd789d 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -89,31 +89,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); }
You can actually just kill off the SetNonBlock method, since we added one to util.h. We should probably do same for SetCloseExec since I believe we have several copies of that with the sme problem too.
@@ -854,8 +842,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")); return ret; }
If you report an error in this scenario then you must also ensure a failure code is returned. This particular scenario is non-fatal though, so I don't think we should be raising an error here at all. I'd be inclined to just keep qemudLog, but with the raw errno value as an int.
@@ -1134,30 +1121,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"));
Likewise with all these - we're just printing ARGV to a logfile - this should not result in errors propagated to the caller - unless we intend to tear down the VM we just started, but I think that's overkill.
@@ -1222,14 +1210,14 @@ 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);
virEventRemoveHandle(vm->monitor_watch);
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; @@ -1384,8 +1372,8 @@ qemudMonitorCommand (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; @@ -1394,8 +1382,8 @@ qemudMonitorCommand (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;
Hmm, these Log -> Error conversions all are same non-fatal scneario I mention earlier.
@@ -1492,7 +1480,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 conversion looks good - we SHOULD be raising a real error here. The original code was wrong to return 'maxvcpus' here, it should be -1 upon error. 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 :|