[libvirt] [PATCH] split out logfile opening
Hi, attached patch splits out the qemu logfile opening into a separate function which makes the code a bit more readable and I'll need this for the libvirtd restart code. Cheers, -- Guido
Guido Günther <agx@sigxcpu.org> wrote:
attached patch splits out the qemu logfile opening into a separate function which makes the code a bit more readable and I'll need this for the libvirtd restart code. ... +static int +qemudLogFD(virConnectPtr conn, const char* logDir, const char* name) +{ + char logfile[PATH_MAX]; + mode_t logmode; + uid_t uid = geteuid(); + int fd = -1; + + if ((strlen(logDir) + /* path */ + 1 + /* Separator */ + strlen(name) + /* basename */ + 4 + /* suffix .log */ + 1 /* NULL */) > PATH_MAX) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("config file path too long: %s/%s.log"), + logDir, name); + return -1; + } + + strcpy(logfile, logDir); + strcat(logfile, "/"); + strcat(logfile, name); + strcat(logfile, ".log");
Since this is just moving code, ok. Otherwise, I'd ask that the strlen check and strcpy/strcat calls be replaced by a single checked snprintf call, using "sizeof logfile" as the length, rather than duplicating PATH_MAX.
On Mon, Jan 05, 2009 at 07:52:26AM +0100, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote:
attached patch splits out the qemu logfile opening into a separate function which makes the code a bit more readable and I'll need this for the libvirtd restart code. ... +static int +qemudLogFD(virConnectPtr conn, const char* logDir, const char* name) +{ + char logfile[PATH_MAX]; + mode_t logmode; + uid_t uid = geteuid(); + int fd = -1; + + if ((strlen(logDir) + /* path */ + 1 + /* Separator */ + strlen(name) + /* basename */ + 4 + /* suffix .log */ + 1 /* NULL */) > PATH_MAX) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("config file path too long: %s/%s.log"), + logDir, name); + return -1; + } + + strcpy(logfile, logDir); + strcat(logfile, "/"); + strcat(logfile, name); + strcat(logfile, ".log");
Since this is just moving code, ok. Otherwise, I'd ask that the strlen check and strcpy/strcat calls be replaced by a single checked snprintf call, using "sizeof logfile" as the length, rather than duplicating PATH_MAX.
I agree with Jim, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Jan 06, 2009 at 03:28:59PM +0100, Daniel Veillard wrote:
On Mon, Jan 05, 2009 at 07:52:26AM +0100, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote:
attached patch splits out the qemu logfile opening into a separate function which makes the code a bit more readable and I'll need this for the libvirtd restart code. ... +static int +qemudLogFD(virConnectPtr conn, const char* logDir, const char* name) +{ + char logfile[PATH_MAX]; + mode_t logmode; + uid_t uid = geteuid(); + int fd = -1; + + if ((strlen(logDir) + /* path */ + 1 + /* Separator */ + strlen(name) + /* basename */ + 4 + /* suffix .log */ + 1 /* NULL */) > PATH_MAX) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("config file path too long: %s/%s.log"), + logDir, name); + return -1; + } + + strcpy(logfile, logDir); + strcat(logfile, "/"); + strcat(logfile, name); + strcat(logfile, ".log");
Since this is just moving code, ok. Otherwise, I'd ask that the strlen check and strcpy/strcat calls be replaced by a single checked snprintf call, using "sizeof logfile" as the length, rather than duplicating PATH_MAX.
I agree with Jim, I only copied the code into a separate function but I can fix this up too while at it. -- Guido
Guido Günther <agx@sigxcpu.org> wrote:
On Tue, Jan 06, 2009 at 03:28:59PM +0100, Daniel Veillard wrote:
On Mon, Jan 05, 2009 at 07:52:26AM +0100, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote:
attached patch splits out the qemu logfile opening into a separate function which makes the code a bit more readable and I'll need this for the libvirtd restart code. ... +static int +qemudLogFD(virConnectPtr conn, const char* logDir, const char* name) +{ + char logfile[PATH_MAX]; + mode_t logmode; + uid_t uid = geteuid(); + int fd = -1; + + if ((strlen(logDir) + /* path */ + 1 + /* Separator */ + strlen(name) + /* basename */ + 4 + /* suffix .log */ + 1 /* NULL */) > PATH_MAX) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("config file path too long: %s/%s.log"), + logDir, name); + return -1; + } + + strcpy(logfile, logDir); + strcat(logfile, "/"); + strcat(logfile, name); + strcat(logfile, ".log");
Since this is just moving code, ok. Otherwise, I'd ask that the strlen check and strcpy/strcat calls be replaced by a single checked snprintf call, using "sizeof logfile" as the length, rather than duplicating PATH_MAX.
I agree with Jim, I only copied the code into a separate function but I can fix this up too while at it.
Thanks! IMHO, committing your patch as-is, and then making the suggested change in a separate change set would be fine. Maybe even a little better, since it's easier to see what's happening when one doesn't mix code-movement changes with "real" changes.
On Tue, Jan 06, 2009 at 04:42:15PM +0100, Jim Meyering wrote: [..snip..]
Thanks! IMHO, committing your patch as-is, and then making the suggested change in a separate change set would be fine. Maybe even a little better, since it's easier to see what's happening when one doesn't mix code-movement changes with "real" changes. Does the attached patch look ok? I'll apply this together with 0001-split-out-opening-of-the-domain-logfile.patch then. -- Guido
On Fri, Jan 09, 2009 at 10:05:06AM +0100, Guido Günther wrote:
On Tue, Jan 06, 2009 at 04:42:15PM +0100, Jim Meyering wrote: [..snip..]
Thanks! IMHO, committing your patch as-is, and then making the suggested change in a separate change set would be fine. Maybe even a little better, since it's easier to see what's happening when one doesn't mix code-movement changes with "real" changes. 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
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; }
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
On Tue, Jan 06, 2009 at 04:42:15PM +0100, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote:
On Tue, Jan 06, 2009 at 03:28:59PM +0100, Daniel Veillard wrote:
On Mon, Jan 05, 2009 at 07:52:26AM +0100, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote:
attached patch splits out the qemu logfile opening into a separate function which makes the code a bit more readable and I'll need this for the libvirtd restart code. ... +static int +qemudLogFD(virConnectPtr conn, const char* logDir, const char* name) +{ + char logfile[PATH_MAX]; + mode_t logmode; + uid_t uid = geteuid(); + int fd = -1; + + if ((strlen(logDir) + /* path */ + 1 + /* Separator */ + strlen(name) + /* basename */ + 4 + /* suffix .log */ + 1 /* NULL */) > PATH_MAX) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("config file path too long: %s/%s.log"), + logDir, name); + return -1; + } + + strcpy(logfile, logDir); + strcat(logfile, "/"); + strcat(logfile, name); + strcat(logfile, ".log");
Since this is just moving code, ok. Otherwise, I'd ask that the strlen check and strcpy/strcat calls be replaced by a single checked snprintf call, using "sizeof logfile" as the length, rather than duplicating PATH_MAX.
I agree with Jim, I only copied the code into a separate function but I can fix this up too while at it.
Thanks! IMHO, committing your patch as-is, and then making the suggested change in a separate change set would be fine. Maybe even a little better, since it's easier to see what's happening when one doesn't mix code-movement changes with "real" changes. Applied now. -- Guido
participants (3)
-
Daniel Veillard -
Guido Günther -
Jim Meyering