Date: Sun, 18 Jul 2010 16:34:35 -0700 From: Garrett Cooper <gcooper@FreeBSD.org> To: Garrett Cooper <gcooper@freebsd.org> Cc: Perforce Change Reviews <perforce@freebsd.org>, Julien Laffaye <jlaffaye@freebsd.org> Subject: Re: PERFORCE change 181154 for review Message-ID: <AANLkTimV7JRwfxZCp-Awk80HMSMTFzZyb9laryh7yjGj@mail.gmail.com> In-Reply-To: <AANLkTim43YyI5TgcmZUDu1g6MOj3nzqpZCiB7awj70gB@mail.gmail.com> References: <201007182159.o6ILxBSq023260@repoman.freebsd.org> <AANLkTim43YyI5TgcmZUDu1g6MOj3nzqpZCiB7awj70gB@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Jul 18, 2010 at 4:18 PM, Garrett Cooper <gcooper@freebsd.org> wrote= : > On Sun, Jul 18, 2010 at 2:59 PM, Julien Laffaye <jlaffaye@freebsd.org> wr= ote: >> http://p4web.freebsd.org/@@181154?ac=3D10 >> >> Change 181154 by jlaffaye@jlaffaye-chulak on 2010/07/18 21:58:13 >> >> =A0 =A0 =A0 =A0Work In Progress!! >> =A0 =A0 =A0 =A0First step using libarchive(3) instead of tar(1). >> =A0 =A0 =A0 =A0The code needs to be more tested, especially with flags s= uch as Fake. >> >> =A0 =A0 =A0 =A0TODO: >> =A0 =A0 =A0 =A0 - Download the file in pkg_do() if it is an url >> =A0 =A0 =A0 =A0 - Read from stdin if the name is "-" in pkg_do() >> =A0 =A0 =A0 =A0 - Re-implement cleanup() >> =A0 =A0 =A0 =A0 - Find packages for dependencies in extract_package() >> =A0 =A0 =A0 =A0 - Add complete package support in pkg_do() >> =A0 =A0 =A0 =A0 - 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 =3D pkg.head; p !=3D NULL; p =3D p->next) { if (p->type =3D=3D PLIST_CONFLICTS) { int i; conflict[0] =3D strdup(p->name); conflict[1] =3D NULL; matched =3D matchinstalled(MATCH_GLOB, conflict, &errcode); free(conflict[0]); if (errcode =3D=3D 0 && matched !=3D NULL) for (i =3D 0; matched[i] !=3D NULL; i++) if (isinstalledpkg(matched[i]) > 0) { warnx("package '%s' conflicts with %s", pkg.name, matched[i]); conflictsfound =3D 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) =3D=3D -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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTimV7JRwfxZCp-Awk80HMSMTFzZyb9laryh7yjGj>