Date: Thu, 1 Apr 2010 17:02:34 -0700 From: Garrett Cooper <yanefbsd@gmail.com> To: Florent Thoumie <flz@freebsd.org> 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 Message-ID: <k2m7d6fde3d1004011702ha697cd7bg96cf6bd8f58babb4@mail.gmail.com> In-Reply-To: <201004011427.o31ERTaT056824@svn.freebsd.org> References: <201004011427.o31ERTaT056824@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Florent and Robert, On Thu, Apr 1, 2010 at 7:27 AM, Florent Thoumie <flz@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?k2m7d6fde3d1004011702ha697cd7bg96cf6bd8f58babb4>