[libvirt] [PATCH] split out logfile opening

[Guido Günther]

On Tue, Jan 06, 2009 at 04:42:15PM +0100, Jim Meyering wrote:
> Guido Günther <agx at 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 at 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