Date: Sun, 18 Jul 2010 18:51:14 -0700 From: Garrett Cooper <gcooper@FreeBSD.org> To: Julien LAFFAYE <jlaffaye@freebsd.org> Cc: Perforce Change Reviews <perforce@freebsd.org> Subject: Re: PERFORCE change 181154 for review Message-ID: <AANLkTinCXsH6oMkRcVUgu_dZBZzHmG2cXMb2tPExwn3E@mail.gmail.com> In-Reply-To: <AANLkTinZOwTDoQhApoGLYDzFcdnkht9q94jFRp2Mmi-p@mail.gmail.com> References: <201007182159.o6ILxBSq023260@repoman.freebsd.org> <AANLkTim43YyI5TgcmZUDu1g6MOj3nzqpZCiB7awj70gB@mail.gmail.com> <AANLkTimV7JRwfxZCp-Awk80HMSMTFzZyb9laryh7yjGj@mail.gmail.com> <AANLkTinZOwTDoQhApoGLYDzFcdnkht9q94jFRp2Mmi-p@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Jul 18, 2010 at 4:45 PM, Julien LAFFAYE <jlaffaye@freebsd.org> wrot=
e:
> On Mon, Jul 19, 2010 at 1:34 AM, Garrett Cooper <gcooper@freebsd.org> wro=
te:
>>
>> 1. Some other things:
>>
>> In add/extract.c:
>>
>> =A0 =A0 =A0 =A0 =A0 =A0for (p =3D pkg.head; p !=3D NULL; p =3D p->next) =
{
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (p->type =3D=3D PLIST_CONFLICTS) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int i;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0conflict[0] =3D strdup(p->name);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0conflict[1] =3D NULL;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0matched =3D matchinstalled(MATCH_=
GLOB, conflict, &errcode);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0free(conflict[0]);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (errcode =3D=3D 0 && matched !=
=3D NULL)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0for (i =3D 0; matched[i] =
!=3D NULL; i++)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (isinstalledpk=
g(matched[i]) > 0) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0warnx("pa=
ckage '%s' conflicts with %s",
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0pkg.name, matched[i]);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0conflicts=
found =3D 1;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>>
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0continue;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>>
>> The continue is useless.
>>
>> 2. These sprintfs have negative value:
>>
>> =A0 =A0 =A0 =A0 =A0 =A0if (fexists(INSTALL_FNAME)) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sprintf(post_script, "%s", INSTALL_FNAME)=
;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sprintf(pre_arg, "PRE-INSTALL");
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sprintf(post_arg, "POST-INSTALL");
>> =A0 =A0 =A0 =A0 =A0 =A0}
>>
>> The last two sprintfs could be implemented with strlcpy (and
>> technically I thought that using sprintf without format strings was
>> stylistically illegal). I don't see why the first one needs to be
>> implemented as a sprintf as it's not user-provided (most of the time
>> that's done to avoid security issues with buffer overruns in the
>> str*cat family, IIRC).
>
> But, but, but! I'm not the original author of this code ;p
> I'll fix that.
I know... the majority of the code can be blamed on other folks :)
(including your's truly).
>> 3. style(9):
>>
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (!Fake && chdir(p->name) =3D=3D -1) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0warn("chdir(%s)", p->name);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 1;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> You mean the lack of () for return ?
Yup.
>> 4. You're leaking the archive objects in many spots by not handling
>> this at function exit:
>>
>> =A0 =A0 =A0 =A0archive_read_finish(a);
>> =A0 =A0 =A0 =A0free_plist(&pkg);
>>
>> =A0 =A0Even though I don't like suggesting this (globals are nasty...
>> ew), maybe the archive object itself should become a global *just in
>> pkg_add* visible from only perform.c, essentially passed via pointer
>> to extract.c, and be handled in the cleanup function in perform.c, so
>> that it gets mopped up when atexit(3) is called. It's a thought I've
>> briefly entertained right now...
>
> That is basically what is planned, except the global part.
> When I'm done (the commit was basically a "snapshot" of the WIP, it's
> not usable), extract_package will take a struct archive * and Package
> * as arguments.
> Both will be created/free'ed in pkg_do().
Ok, good to hear then. Just trying to help out with an extra pair of eyes.
-Garrett
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTinCXsH6oMkRcVUgu_dZBZzHmG2cXMb2tPExwn3E>