On Sun, Jan 11, 2009 at 08:50:59AM +0100, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote:
Does the attached patch look ok? I'll apply this together with 0001-split-out-opening-of-the-domain-logfile.patch then. -- Guido Rereading your mail you also suggest using sizeof() instead of PATH_MAX. Updated patch attached. -- Guido
From 6ca6494be05e4834b9469ec1c8a108cefe3ed44f Mon Sep 17 00:00:00 2001 From: =?utf-8?q?Guido=20G=C3=BCnther?= <agx@sigxcpu.org> Date: Sat, 10 Jan 2009 20:13:39 +0100 Subject: [PATCH] use snprintf
--- src/qemu_driver.c | 19 ++++++++----------- 1 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index d4c56d6..fc73a12 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -149,24 +149,21 @@ qemudLogFD(virConnectPtr conn, const char* logDir, const char* name) char logfile[PATH_MAX]; mode_t logmode; uid_t uid = geteuid(); - int fd = -1; + int ret, fd = -1;
- if ((strlen(logDir) + /* path */ - 1 + /* Separator */ - strlen(name) + /* basename */ - 4 + /* suffix .log */ - 1 /* NULL */) > PATH_MAX) { + if ((ret = snprintf(logfile, sizeof(logfile), "%s/%s.log", logDir, name)) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("failed to build logfile name %s/%s.log"), + logDir, name); + return -1; + } + if (ret >= sizeof(logfile)) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("config file path too long: %s/%s.log"), logDir, name); return -1; }
Thanks! That looks fine. However, it makes me see the 2nd diagnostic is wrong to mention "config file". Should be "log file". Yet again, this was a preexisting error, not yours, of course.
Maybe just drop the "path too long" case (which will probably never happen anyway) and combine the tests, also splitting the line to fit in 80 columns?
if ((ret = snprintf(logfile, sizeof(logfile), "%s/%s.log", logDir, name)) < 0 || ret >= sizeof(logfile)) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("failed to build logfile name %s/%s.log"), logDir, name); return -1; } That looks even better. Applied that way. -- Guido