From owner-p4-projects@FreeBSD.ORG Sun Jul 18 23:45:52 2010 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id D3E731065673; Sun, 18 Jul 2010 23:45:51 +0000 (UTC) Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9861F106564A for ; Sun, 18 Jul 2010 23:45:51 +0000 (UTC) (envelope-from julien.laffaye@gmail.com) Received: from mail-bw0-f54.google.com (mail-bw0-f54.google.com [209.85.214.54]) by mx1.freebsd.org (Postfix) with ESMTP id EA9B08FC13 for ; Sun, 18 Jul 2010 23:45:50 +0000 (UTC) Received: by bwz12 with SMTP id 12so2349447bwz.13 for ; Sun, 18 Jul 2010 16:45:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:sender:received :in-reply-to:references:date:x-google-sender-auth:message-id:subject :from:to:cc:content-type:content-transfer-encoding; bh=Bl7YO27oKH0p/N7fByPGkXKxERmXVXQ+i7A8arIuKA0=; b=Cx1ruFlHxXFXYQQbjqcPouJKLGyeKjqbU06Mux5p/hPIke1y41LSgMTCE27npG4sp1 umd9WefZzz4VSgtwGQJnF0cxJwDKhJgWF+d+K3mABv4PwYmyyksdfLLu4Pjd+IgDYFox S/XFSddWcMQUzM2asS816dqf9lBgyocELcXRY= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=MflInlghbMShejVaCslkherHllFmFnxkkPOfnap82H/WoNWHTd91+bTnzkg8Ak/NR1 7SFtAVDdaoDK4/3l04zHt+E6aXR/Q0T7f7BplN+/czVZpKCcbQDBA6qzd/dEoq4q2bWh tczFjIBmRcBXzRoqPjCZZ9GmOPkIKLRlQPxHU= MIME-Version: 1.0 Received: by 10.204.79.223 with SMTP id q31mr2929179bkk.92.1279496749586; Sun, 18 Jul 2010 16:45:49 -0700 (PDT) Sender: julien.laffaye@gmail.com Received: by 10.204.62.75 with HTTP; Sun, 18 Jul 2010 16:45:49 -0700 (PDT) In-Reply-To: References: <201007182159.o6ILxBSq023260@repoman.freebsd.org> Date: Mon, 19 Jul 2010 01:45:49 +0200 X-Google-Sender-Auth: rJZwiIC5fSWfmdn2zYinwbgxcx8 Message-ID: From: Julien LAFFAYE To: Garrett Cooper Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Perforce Change Reviews Subject: Re: PERFORCE change 181154 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 18 Jul 2010 23:45:52 -0000 On Mon, Jul 19, 2010 at 1:34 AM, Garrett Cooper wrote= : > > 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_G= LOB, 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 (isinstalledpkg= (matched[i]) > 0) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0warnx("pac= kage '%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 =A0conflictsf= ound =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. > > 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 ? > > 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(). > > Thanks, > -Garrett >