Re: [libvirt] [PATCH] split out logfile opening

Guido Günther <agx(a)sigxcpu.org&gt; 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(a)sigxcpu.org&gt; 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.