[libvirt] [PATCH] eliminate strerror from qemu_driver.c: use virReportSystemError instead
I've begun eliminating the remaining problematic uses of strerror. For example, those in virsh.c aren't a problem. This is required for thread safety. After this patch, there are about 60 uses left. Note while reviewing: - are there places where I've added uses of "conn" that I should not have? - are there places where I've used NULL but it should be "conn"? (there may well be -- I haven't tried hard, but that matters less) - I've assumed that qemudEscapeMonitorArg failure will always be due to OOM Currently it is. I removed the calls to qemudLog from each of the qemudSet* functions because now each caller of those functions already diagnoses failure, and includes strerror(errno) information, while the qemudLog calls did not. In addition, the eliminated qemudLog calls would clobber errno, making each callers' attempt to report strerror(errno) use a potentially invalid and misleading errno value.
From ebe2ee1fbf20edbb07a78ecce76de6866e2ef558 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 28 Jan 2009 19:20:08 +0100 Subject: [PATCH] eliminate strerror from qemu_driver.c: use virReportSystemError instead
* src/qemu_driver.c (qemudSetCloseExec): Don't use qemudLog here. Now, every caller diagnoses the failure. Simplify, now that there's no logging. * src/qemu_driver.c (qemudSetNonBlock): Rewrite not to use qemudLog. --- src/qemu_driver.c | 126 ++++++++++++++++++++++++----------------------------- 1 files changed, 57 insertions(+), 69 deletions(-) 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); } @@ -198,22 +186,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; @@ -441,8 +428,9 @@ qemudStartup(void) { } if (virFileMakePath(qemu_driver->stateDir) < 0) { - qemudLog(QEMUD_ERR, _("Failed to create state dir '%s': %s\n"), - qemu_driver->stateDir, strerror(errno)); + virReportSystemError(NULL, errno, + _("Failed to create state dir '%s'"), + qemu_driver->stateDir); goto error; } @@ -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; } @@ -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")); for (i = 0 ; i < ntapfds ; i++) FD_SET(tapfds[i], &keepfd); @@ -1214,7 +1201,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; @@ -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; @@ -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; } @@ -1735,8 +1723,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. */ @@ -3821,8 +3809,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; } @@ -3832,8 +3820,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; } @@ -3887,8 +3875,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; } @@ -3904,8 +3892,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; } @@ -4064,8 +4053,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; } @@ -4235,8 +4224,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.418.gd79e6
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 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c 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
On Thu, Jan 29, 2009 at 09:54:59PM +0100, Jim Meyering wrote: problem too.
Ah. I see that now. Have done both things.
@@ -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
Why? because of semantics implied by the "Error" suffix? Then maybe we need a *Warn function with similar functionality.
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.
Why not? If I get EIO or ENOSPC, I'd like to see diagnostics in the logs ASAP. The closer to the point of origin the better. If we keep using qemudLog, then I'll just make it use virStrerror or something along those lines. there's no need to print raw errno values.
@@ -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.
But failure still means write errors. It doesn't matter so much what we're writing as *that* there's been a relatively serious (write) error. Not reporting a write error now could easily mask or delay reporting a later one involving important data. That said, I don't feel very strongly either way, so tell me what you'd prefer.
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.
I've just converted another that I'll merge onto the rest:
From d02669442e0cd2354e6b429dbedcf665d15cfa87 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 30 Jan 2009 23:18:04 +0100 Subject: [PATCH] report OOMError
--- src/qemu_driver.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index c40fda4..599af0b 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -462,8 +462,7 @@ qemudStartup(void) { return 0; out_of_memory: - qemudLog (QEMUD_ERR, - "%s", _("qemudStartup: out of memory\n")); + virReportOOMError(NULL); error: if (qemu_driver) qemuDriverUnlock(qemu_driver); -- 1.6.1.2.418.gd79e6
Jim Meyering <jim@meyering.net> wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c 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
On Thu, Jan 29, 2009 at 09:54:59PM +0100, Jim Meyering wrote: problem too.
FYI, here's how I am now removing uses of strerror. This is less invasive than what I proposed initially, and (importantly) it's far easier to automate the transformation. I'll post the full patch soon. Jim
From 27724d561859da6a711cfc62b04fe1a3c074b833 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 2 Feb 2009 11:26:33 +0100 Subject: [PATCH] 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.443.g0d7c2
On Mon, Feb 02, 2009 at 11:38:33AM +0100, Jim Meyering wrote:
Jim Meyering <jim@meyering.net> wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c 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
On Thu, Jan 29, 2009 at 09:54:59PM +0100, Jim Meyering wrote: problem too.
FYI, here's how I am now removing uses of strerror. This is less invasive than what I proposed initially, and (importantly) it's far easier to automate the transformation.
I'll post the full patch soon.
This seems like a reasonable approach 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 :|
participants (2)
-
Daniel P. Berrange -
Jim Meyering