PERFORCE change 181154 for review

[Garrett Cooper]

On Sun, Jul 18, 2010 at 4:45 PM, Julien LAFFAYE <jlaffaye at freebsd.org> wrote:
> On Mon, Jul 19, 2010 at 1:34 AM, Garrett Cooper <gcooper at 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