Guido Günther <agx@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list