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>
index | next in thread | previous in thread | raw e-mail
On Sun, Jul 18, 2010 at 4:45 PM, Julien LAFFAYE <jlaffaye@freebsd.org> wrote:
> On Mon, Jul 19, 2010 at 1:34 AM, Garrett Cooper <gcooper@freebsd.org> wrote:
>>
>> 1. Some other things:
>>
>> In add/extract.c:
>>
>> for (p = pkg.head; p != NULL; p = p->next) {
>> if (p->type == PLIST_CONFLICTS) {
>> int i;
>> conflict[0] = strdup(p->name);
>> conflict[1] = NULL;
>> matched = matchinstalled(MATCH_GLOB, conflict, &errcode);
>> free(conflict[0]);
>> if (errcode == 0 && matched != NULL)
>> for (i = 0; matched[i] != NULL; i++)
>> if (isinstalledpkg(matched[i]) > 0) {
>> warnx("package '%s' conflicts with %s",
>> pkg.name, matched[i]);
>> conflictsfound = 1;
>> }
>>
>> continue;
>> }
>>
>> The continue is useless.
>>
>> 2. These sprintfs have negative value:
>>
>> if (fexists(INSTALL_FNAME)) {
>> sprintf(post_script, "%s", INSTALL_FNAME);
>> sprintf(pre_arg, "PRE-INSTALL");
>> sprintf(post_arg, "POST-INSTALL");
>> }
>>
>> 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):
>>
>> if (!Fake && chdir(p->name) == -1) {
>> warn("chdir(%s)", p->name);
>> return 1;
>> }
> 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:
>>
>> archive_read_finish(a);
>> free_plist(&pkg);
>>
>> Even 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
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTinCXsH6oMkRcVUgu_dZBZzHmG2cXMb2tPExwn3E>