[libvirt] [PATCH] split out logfile opening

[Guido Günther]

On Sun, Jan 11, 2009 at 08:50:59AM +0100, Jim Meyering wrote:
> Guido Günther <agx at 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 at 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