On Sun, Jan 11, 2009 at 08:50:59AM +0100, Jim Meyering wrote:
> Guido Günther <agx@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > 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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list