2:54 p.m.
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(a)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
8:59 a.m.
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 :|
4:38 p.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
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
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.
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(a)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
4:38 a.m.
Jim Meyering <jim(a)meyering.net> wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> 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
> 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.
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(a)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
9:50 a.m.
On Mon, Feb 02, 2009 at 11:38:33AM +0100, Jim Meyering wrote:
Jim Meyering <jim(a)meyering.net> wrote:
> "Daniel P. Berrange" <berrange(a)redhat.com> wrote:
>> 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
>> 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.
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 :|
5806
days inactive
5810
days old
4 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Jim Meyering