From owner-svn-src-head@FreeBSD.ORG Fri Apr 2 00:02:36 2010 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 7A043106566C; Fri, 2 Apr 2010 00:02:36 +0000 (UTC) (envelope-from yanefbsd@gmail.com) Received: from mail-qy0-f199.google.com (mail-qy0-f199.google.com [209.85.221.199]) by mx1.freebsd.org (Postfix) with ESMTP id C78338FC08; Fri, 2 Apr 2010 00:02:35 +0000 (UTC) Received: by qyk37 with SMTP id 37so1813548qyk.8 for ; Thu, 01 Apr 2010 17:02:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:received:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=T/Ani2z6IV/skKn8Gsg9shES/5oLmzedMMrbNCzV/ro=; b=S81HtxBnZo6ZbtR093S80HfIroP0YCNQFgmgHhDB4KiDbh8+eK1xqo5zpNjG6soswc kXeFjpA79rzU9t71PLMMGeBxyeDy0MK2Idtq5O2RFhAKLzbjH08RaRDVq1iOjJdkAQB9 braRKV4z5DKNGMsBulNjG4gKysTx82g2Qqr3I= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=xhUTLKNWtbSGrKW+pFGxB2zvpS9v5as0PPrHOUFNAqBnQYVAi7BKf5HKyy4Juqg5J+ 7p8nfrK0eCdTocRGHNVAf7h5MjU1c4gbtQN4N1CInD5rdMtWT8sx2ZXDZjECPpbO4qNr n4ZxB+yi85RzgfTnN+H3kuXnPZrGziXJ8A4Y8= MIME-Version: 1.0 Received: by 10.229.33.72 with HTTP; Thu, 1 Apr 2010 17:02:34 -0700 (PDT) In-Reply-To: <201004011427.o31ERTaT056824@svn.freebsd.org> References: <201004011427.o31ERTaT056824@svn.freebsd.org> Date: Thu, 1 Apr 2010 17:02:34 -0700 Received: by 10.229.222.205 with SMTP id ih13mr2557432qcb.73.1270166554849; Thu, 01 Apr 2010 17:02:34 -0700 (PDT) Message-ID: From: Garrett Cooper To: Florent Thoumie Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, rwatson@freebsd.org Subject: Re: svn commit: r206043 - in head/usr.sbin/pkg_install: add delete lib version X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Apr 2010 00:02:36 -0000 Hi Florent and Robert, On Thu, Apr 1, 2010 at 7:27 AM, Florent Thoumie wrote: > Author: flz > Date: Thu Apr =A01 14:27:29 2010 > New Revision: 206043 > URL: http://svn.freebsd.org/changeset/base/206043 > > Log: > =A0Various fixes. > > =A0- Replace hardcoded INDEX version. [1] > =A0- Fix a buffer overlap. [2] > =A0- Remove empty package when fetching fails and -K is used. [3] > =A0- Remove useless chmod2() after mkdtemp(3). [4] > =A0- Replace mkdir(1) call with mkdir(2). [5] > =A0- Get rid of some vsystem() calls. > =A0- Switch from lstat(2) to open(2) in fexists(). > =A0- Try rename(2) in move_file() first. > =A0- Bump PKG_INSTALL_VERSION to 20100401. > > =A0PR: =A0 =A0 =A0 =A0 =A0 bin/145101 [1], bin/139492 [2], bin/144919 [3] > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bin/144920 [4], bin/144921 [5] > =A0Submitted by: gcooper [1,2,3,4,5] > > Modified: > =A0head/usr.sbin/pkg_install/add/futil.c > =A0head/usr.sbin/pkg_install/add/perform.c > =A0head/usr.sbin/pkg_install/delete/perform.c > =A0head/usr.sbin/pkg_install/lib/file.c > =A0head/usr.sbin/pkg_install/lib/lib.h > =A0head/usr.sbin/pkg_install/lib/match.c > =A0head/usr.sbin/pkg_install/lib/pen.c > =A0head/usr.sbin/pkg_install/lib/url.c > =A0head/usr.sbin/pkg_install/version/perform.c [...] > Modified: head/usr.sbin/pkg_install/add/perform.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- head/usr.sbin/pkg_install/add/perform.c =A0 =A0 Thu Apr =A01 13:27:27= 2010 =A0 =A0 =A0 =A0(r206042) > +++ head/usr.sbin/pkg_install/add/perform.c =A0 =A0 Thu Apr =A01 14:27:29= 2010 =A0 =A0 =A0 =A0(r206043) > @@ -78,6 +78,7 @@ pkg_do(char *pkg) > =A0 =A0 char pre_arg[FILENAME_MAX], post_arg[FILENAME_MAX]; > =A0 =A0 char *conflict[2]; > =A0 =A0 char **matched; > + =A0 =A0int fd; [...] > =A0 =A0 =A0 =A0/* Make sure pkg_info can read the entry */ > - =A0 =A0 =A0 vsystem("/bin/chmod a+rx %s", LogDir); > + =A0 =A0 =A0 fd =3D open(LogDir, O_RDWR); > + =A0 =A0 =A0 fstat(fd, &sb); > + =A0 =A0 =A0 fchmod(fd, sb.st_mode | S_IRALL | S_IXALL); =A0 =A0 /* be s= ure, chmod a+rx */ > + =A0 =A0 =A0 close(fd); Yipes... we really should be checking to make sure that fchmod, fstat, and open pass. This would look really bad if not. I would also argue that all of the files that aren't properly chmod'ed, etc are in fact the packager's fault and should be fixed by adding appropriate scripts because mtree should catch this stuff if it's setup appropriately. [...] > Modified: head/usr.sbin/pkg_install/lib/file.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- head/usr.sbin/pkg_install/lib/file.c =A0 =A0 =A0 =A0Thu Apr =A01 13:2= 7:27 2010 =A0 =A0 =A0 =A0(r206042) > +++ head/usr.sbin/pkg_install/lib/file.c =A0 =A0 =A0 =A0Thu Apr =A01 14:2= 7:29 2010 =A0 =A0 =A0 =A0(r206043) > @@ -31,10 +31,13 @@ __FBSDID("$FreeBSD$"); > =A0Boolean > =A0fexists(const char *fname) > =A0{ > - =A0 =A0struct stat dummy; > - =A0 =A0if (!lstat(fname, &dummy)) > - =A0 =A0 =A0 return TRUE; > - =A0 =A0return FALSE; > + =A0 =A0int fd; > + > + =A0 =A0if ((fd =3D open(fname, O_RDONLY)) =3D=3D -1) > + =A0 =A0 =A0 return FALSE; > + > + =A0 =A0close(fd); > + =A0 =A0return TRUE; > =A0} I was leery of this change because fexists is used widely across pkg_install for purposes other than just determining whether or not a file existed, in particular it's determining whether or not the end-file exists. The original submitter for the PR didn't actually test this point thoroughly so I need to go and write some tests to ensure that the code isn't actually regressing on accident :/. > =A0/* Quick check to see if something is a directory or symlink to a dire= ctory */ > @@ -279,17 +282,23 @@ copy_file(const char *dir, const char *f > =A0} > > =A0void > -move_file(const char *dir, const char *fname, const char *to) > +move_file(const char *dir, const char *fname, const char *tdir) > =A0{ > - =A0 =A0char cmd[FILENAME_MAX]; > + =A0 =A0char from[FILENAME_MAX]; > + =A0 =A0char to[FILENAME_MAX]; > > =A0 =A0 if (fname[0] =3D=3D '/') > - =A0 =A0 =A0 snprintf(cmd, FILENAME_MAX, "/bin/mv %s %s", fname, to); > + =A0 =A0 =A0 strncpy(from, fname, FILENAME_MAX); > =A0 =A0 else > - =A0 =A0 =A0 snprintf(cmd, FILENAME_MAX, "/bin/mv %s/%s %s", dir, fname,= to); > - =A0 =A0if (vsystem(cmd)) { > - =A0 =A0 =A0 cleanup(0); > - =A0 =A0 =A0 errx(2, "%s: could not perform '%s'", __func__, cmd); > + =A0 =A0 =A0 snprintf(from, FILENAME_MAX, "%s/%s", dir, fname); > + > + =A0 =A0snprintf(to, FILENAME_MAX, "%s/%s", tdir, fname); > + > + =A0 =A0if (rename(from, to) =3D=3D -1) { > + =A0 =A0 =A0 =A0if (vsystem("/bin/mv %s %s", from, to)) { > + =A0 =A0 =A0 =A0 =A0 cleanup(0); > + =A0 =A0 =A0 =A0 =A0 errx(2, "%s: could not move '%s' to '%s'", __func__= , from, to); > + =A0 =A0 =A0 } > =A0 =A0 } > =A0} 1. FILENAME_MAX could be less than PATH_MAX, and actually is on the BSDs (256 vs 1024). PATH_MAX allows for duplicate slashes and all sorts of whacky path crud and probably should be used more liberally in the pkg_install code. This however isn't always true in the NetBSD case because they're aiming for portability of pkg_install, however PATH_MAX is always guaranteed to be at least as large as FILENAME_MAX. 2. Does rename(2) copy [MAC] and other attributes properly? It appears to do the right thing, but I could be misreading the code... Thanks for the commits BTW :)! -Garrett