PERFORCE change 181154 for review

[Garrett Cooper]

On Sun, Jul 18, 2010 at 4:18 PM, Garrett Cooper <gcooper at freebsd.org> wrote:
> On Sun, Jul 18, 2010 at 2:59 PM, Julien Laffaye <jlaffaye at freebsd.org> wrote:
>> http://p4web.freebsd.org/@@181154?ac=10
>>
>> Change 181154 by jlaffaye at jlaffaye-chulak on 2010/07/18 21:58:13
>>
>>        Work In Progress!!
>>        First step using libarchive(3) instead of tar(1).
>>        The code needs to be more tested, especially with flags such as Fake.
>>
>>        TODO:
>>         - Download the file in pkg_do() if it is an url
>>         - Read from stdin if the name is "-" in pkg_do()
>>         - Re-implement cleanup()
>>         - Find packages for dependencies in extract_package()
>>         - Add complete package support in pkg_do()
>>         - Slave/Master mode?! in pkg_do()
>
> Wouldn't it make more sense for extract_package to be in libpkg,
> because it's basically a utility function that would be used by
> pkg_add and pkg_complete?
>
> Also, Tim and I discussed initializing the decompressor only once
> because it would greatly simplify the code in libpkg today, and would
> eliminate wasted CPU cycles used when initializing the decompressor
> each time a metadata file is extracted from the archive object.
>
> Sorry to be a wanker on this too, but considering that all of the code
> moved over from extract.c is basically `new code', could it be cleaned
> up for style(9)?

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).

3. style(9):

		if (!Fake && chdir(p->name) == -1) {
		    warn("chdir(%s)", p->name);
		    return 1;
		}

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...

Thanks,
-Garrett