[pacman-dev] Removing directory symlink support
The bugs in 4.1 are mostly addressed and I will make a 4.1.1 release sometime soon, so now is time to think about pacman-4.2. I wanted to discuss the removal of directory symlink support again because there was not much response on the list last time. Currently, if there is a symlink to a directory on the filesystem, we can treat it like a directory. e.g. Arch Linux has /lib -> usr/lib. So if a package installs a file in /lib, pacman silently puts it in /usr/lib. I'd like to remove that support for the following reasons: 1) It requires we resolve the true path of every directory involved in an upgrade for conflict checking. e.g. /lib/foo.so and /usr/lib/foo.so are conflicts. This has substantial overhead, both in terms of computing and in code. Removing this would mean all symlinks would be treated like a file. 2) We do not handle it very well. e.g. if a package contains a /foo -> some/dir symlink, it needs to be installed before any package with a file in /foo in the same transaction. We "implement" this through flagging a conflict. When the package containing the directory symlink is removed, we need to scan all packages to ensure it is not being "used" by any other package in the local database. This is covered by the _failing_ pactests sync700-702. 3) If packages really want to install files in a directory where there is a symlink, it is easily dealt with in the PKGBUILD. e.g. package() { ln -s usr/lib $pkgdir/lib make DESTDIR=$pkgdir rm $pkgdir/lib } 4) There is no use case for it (at least on linux). I have only ever seen this used as a poor man's bind mount. 5) It believe it would actually make replacing /bin and /sbin able to occur in a single transaction (no --ignore filesystem) as we can replace a completely removed directory by a file. Any comments here? I see there is a patchset already available that does most (all?) of the removal. What we would need is to have something like a pacman-db-upgrade that changed filelists to avoid symlink paths. E.g a package with /lib/foo haves its filelist adjusted to /usr/lib/foo. The other option is to have testdb check for these and advise people to run that before updating to pacman-4.2? Any comments? Allan
Allan McRae <allan@archlinux.org> on Fri, 2013/04/12 15:59:
5) It believe it would actually make replacing /bin and /sbin able to occur in a single transaction (no --ignore filesystem) as we can replace a completely removed directory by a file.
So completing the /usr move is delayed until pacman 4.2 is released? -- main(a){char*c=/* Schoene Gruesse */"B?IJj;MEH" "CX:;",b;for(a/* Chris get my mail address: */=0;b=c[a++];) putchar(b-1/(/* gcc -o sig sig.c && ./sig */b/42*2-3)*42);}
On 04/12/13 at 03:59pm, Allan McRae wrote:
I wanted to discuss the removal of directory symlink support again because there was not much response on the list last time. <snip> 5) It believe it would actually make replacing /bin and /sbin able to occur in a single transaction (no --ignore filesystem) as we can replace a completely removed directory by a file.
Sadly, this is not the case. In order for that to work, the directory to file transition logic in conflict.c would have to be able to take the packages being upgraded into account, which would require more complex transaction ordering logic than what we have now. Adding packages being removed as conflicts to the dir->file logic, however, was pretty easy once symlinks were removed, so I've added that, which fixes FS#32770. To better illustrate the effect removing symlink support has, here are the tests whose results changed from my no-symlink branch. --- fileconflict013.py | 4 ++-- fileconflict023.py | 6 +++--- fileconflict025.py | 6 +++--- symlink001.py | 9 ++++----- sync700.py | 9 ++++----- sync701.py | 9 ++++----- sync702.py | 9 ++++----- 10 files changed, 42 insertions(+), 29 deletions(-) diff --git a/test/pacman/tests/fileconflict013.py b/test/pacman/tests/fileconflict013.py index a83923c..94c169d 100644 --- a/test/pacman/tests/fileconflict013.py +++ b/test/pacman/tests/fileconflict013.py @@ -1,20 +1,20 @@ self.description = "file->file path change with same effective path (/lib as symlink)" lp1 = pmpkg("filesystem", "1.0-1") lp1.files = ["usr/", "usr/lib/", "lib -> usr/lib/"] self.addpkg2db("local", lp1) lp2 = pmpkg("pkg1", "1.0-1") lp2.files = ["lib/libfoo.so"] self.addpkg2db("local", lp2) sp1 = pmpkg("pkg1", "1.0-2") sp1.files = ["usr/lib/libfoo.so"] self.addpkg2db("sync", sp1) self.args = "-Su" -self.addrule("PACMAN_RETCODE=0") -self.addrule("PKG_VERSION=pkg1|1.0-2") +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_VERSION=pkg1|1.0-1") diff --git a/test/pacman/tests/fileconflict023.py b/test/pacman/tests/fileconflict023.py index ce72087..1367080 100644 --- a/test/pacman/tests/fileconflict023.py +++ b/test/pacman/tests/fileconflict023.py @@ -1,18 +1,18 @@ self.description = "File conflict between package with symlink and package with real path resolved by removal" self.filesystem = ["usr/", "usr/lib/", "lib -> usr/lib/"] lp1 = pmpkg("foo") lp1.files = ["lib/", "lib/file"] self.addpkg2db("local", lp1) sp1 = pmpkg("bar") sp1.conflicts = ["foo"] sp1.files = ["usr/", "usr/lib/", "usr/lib/file"] self.addpkg2db("sync", sp1) self.args = "-S %s --ask=4" % sp1.name -self.addrule("PACMAN_RETCODE=0") -self.addrule("!PKG_EXIST=foo") -self.addrule("PKG_EXIST=bar") +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_EXIST=foo") +self.addrule("!PKG_EXIST=bar") diff --git a/test/pacman/tests/fileconflict025.py b/test/pacman/tests/fileconflict025.py index 3c6f063..013c223 100644 --- a/test/pacman/tests/fileconflict025.py +++ b/test/pacman/tests/fileconflict025.py @@ -1,18 +1,18 @@ self.description = "File conflict between package with symlink and package with real path resolved by removal (reversed)" self.filesystem = ["usr/lib/", "lib -> usr/lib/"] lp1 = pmpkg("foo") lp1.files = ["usr/", "usr/lib/", "usr/lib/file"] self.addpkg2db("local", lp1) sp1 = pmpkg("bar") sp1.conflicts = ["foo"] sp1.files = ["lib/", "lib/file"] self.addpkg2db("sync", sp1) self.args = "-S %s --ask=4" % sp1.name -self.addrule("PACMAN_RETCODE=0") -self.addrule("!PKG_EXIST=foo") -self.addrule("PKG_EXIST=bar") +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_EXIST=foo") +self.addrule("!PKG_EXIST=bar") diff --git a/test/pacman/tests/symlink001.py b/test/pacman/tests/symlink001.py index cbf71cc..9136551 100644 --- a/test/pacman/tests/symlink001.py +++ b/test/pacman/tests/symlink001.py @@ -1,20 +1,19 @@ self.description = "Dir symlinks overwritten on install (the perl/git bug)" lp = pmpkg("dummy") lp.files = ["dir/realdir/", "dir/symdir -> realdir"] self.addpkg2db("local", lp) p = pmpkg("pkg1") p.files = ["dir/symdir/tmp"] self.addpkg(p) self.args = "-U %s" % p.filename() -self.addrule("PACMAN_RETCODE=0") -self.addrule("PKG_EXIST=pkg1") -self.addrule("FILE_EXIST=dir/symdir/tmp") -self.addrule("FILE_EXIST=dir/realdir/tmp") -self.addrule("FILE_TYPE=dir/symdir/tmp|file") +self.addrule("PACMAN_RETCODE=1") +self.addrule("!PKG_EXIST=pkg1") +self.addrule("!FILE_EXIST=dir/symdir/tmp") +self.addrule("!FILE_EXIST=dir/realdir/tmp") self.addrule("FILE_TYPE=dir/symdir|link") self.addrule("FILE_TYPE=dir/realdir|dir") diff --git a/test/pacman/tests/sync700.py b/test/pacman/tests/sync700.py index 9748c81..8e908e0 100644 --- a/test/pacman/tests/sync700.py +++ b/test/pacman/tests/sync700.py @@ -1,22 +1,21 @@ self.description = "do not remove directory symlink if another package has file in its path" lp1 = pmpkg("pkg1") lp1.files = ["usr/lib/foo", "lib -> usr/lib"] self.addpkg2db("local", lp1) lp2 = pmpkg("pkg2") lp2.files = ["lib/bar"] self.addpkg2db("local", lp2) p = pmpkg("pkg1", "1.0-2") p.files = ["usr/lib/foo"] self.addpkg2db("sync", p) self.args = "-S pkg1" -self.addrule("PACMAN_RETCODE=1") -self.addrule("PKG_VERSION=pkg1|1.0-1") -self.addrule("FILE_EXIST=lib/bar") - -self.expectfailure = True +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=pkg1|1.0-2") +self.addrule("FILE_EXIST=usr/lib/bar") +self.addrule("!FILE_EXIST=lib/bar") diff --git a/test/pacman/tests/sync701.py b/test/pacman/tests/sync701.py index 201f602..977b1a0 100644 --- a/test/pacman/tests/sync701.py +++ b/test/pacman/tests/sync701.py @@ -1,22 +1,21 @@ self.description = "do not remove directory symlink if incoming package has file in its path (order 1)" lp = pmpkg("pkg1") lp.files = ["usr/lib/foo", "lib -> usr/lib"] self.addpkg2db("local", lp) p1 = pmpkg("pkg1", "1.0-2") p1.files = ["usr/lib/foo"] self.addpkg2db("sync", p1) p2 = pmpkg("pkg2") p2.files = ["lib/bar"] self.addpkg2db("sync", p2) self.args = "-S pkg1 pkg2" -self.addrule("PACMAN_RETCODE=1") -self.addrule("PKG_VERSION=pkg1|1.0-1") -self.addrule("!PKG_EXIST=pkg2") - -self.expectfailure = True +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=pkg1|1.0-2") +self.addrule("PKG_EXIST=pkg2") +self.addrule("FILE_TYPE=lib|dir") diff --git a/test/pacman/tests/sync702.py b/test/pacman/tests/sync702.py index ee4eef9..a7904c3 100644 --- a/test/pacman/tests/sync702.py +++ b/test/pacman/tests/sync702.py @@ -1,22 +1,21 @@ self.description = "do not remove directory symlink if incoming package has file in its path (order 2)" lp = pmpkg("pkg2") lp.files = ["usr/lib/foo", "lib -> usr/lib"] self.addpkg2db("local", lp) p1 = pmpkg("pkg1") p1.files = ["lib/bar"] self.addpkg2db("sync", p1) p2 = pmpkg("pkg2", "1.0-2") p2.files = ["usr/lib/foo"] self.addpkg2db("sync", p2) self.args = "-S pkg1 pkg2" -self.addrule("PACMAN_RETCODE=1") -self.addrule("PKG_VERSION=pkg2|1.0-1") -self.addrule("!PKG_EXIST=pkg1") - -self.expectfailure = True +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=pkg2|1.0-2") +self.addrule("PKG_EXIST=pkg1") +self.addrule("FILE_TYPE=lib|dir")
Two weeks with no objections, so here is the full patchset. Several of the tests should be redundant now, but I left them in with updated rules anyway, just in case. Also available on my no-symlinks branch for those interested. Andrew Gregory (8): update tests for symlink support removal alpm_filelist: remove resolved_path conflict.c: do not ignore symlink<->dir conflicts conflict.c: use real path for filesystem checks extract_single_file: consolidate symlink cases unlink_file: treat symlinks like normal files query_fileowner: remove symlink support improve dir->file transition conflict resolution lib/libalpm/add.c | 33 ++--- lib/libalpm/alpm.h | 3 +- lib/libalpm/conflict.c | 192 +++++++++++++----------------- lib/libalpm/filelist.c | 225 ++--------------------------------- lib/libalpm/filelist.h | 4 - lib/libalpm/package.c | 22 +--- lib/libalpm/remove.c | 11 +- src/common/util-common.c | 43 +++++++ src/common/util-common.h | 1 + src/pacman/query.c | 90 +++----------- test/pacman/tests/fileconflict007.py | 1 + test/pacman/tests/fileconflict013.py | 5 +- test/pacman/tests/fileconflict022.py | 1 - test/pacman/tests/fileconflict023.py | 7 +- test/pacman/tests/fileconflict025.py | 8 +- test/pacman/tests/fileconflict030.py | 17 +++ test/pacman/tests/symlink001.py | 9 +- test/pacman/tests/sync700.py | 12 +- test/pacman/tests/sync701.py | 11 +- test/pacman/tests/sync702.py | 11 +- 20 files changed, 217 insertions(+), 489 deletions(-) create mode 100644 test/pacman/tests/fileconflict030.py -- 1.8.2.1
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- test/pacman/tests/fileconflict007.py | 1 + test/pacman/tests/fileconflict013.py | 7 +++++-- test/pacman/tests/fileconflict022.py | 1 - test/pacman/tests/fileconflict023.py | 9 ++++++--- test/pacman/tests/fileconflict025.py | 10 ++++++---- test/pacman/tests/symlink001.py | 11 ++++++----- test/pacman/tests/sync700.py | 12 ++++++------ test/pacman/tests/sync701.py | 11 +++++------ test/pacman/tests/sync702.py | 9 +++++---- 9 files changed, 40 insertions(+), 31 deletions(-) diff --git a/test/pacman/tests/fileconflict007.py b/test/pacman/tests/fileconflict007.py index 4ee4624..7fe65ed 100644 --- a/test/pacman/tests/fileconflict007.py +++ b/test/pacman/tests/fileconflict007.py @@ -1,17 +1,18 @@ self.description = "Fileconflict with symlinks (klibc case)" lp = pmpkg("pkg") lp.files = ["dir/realdir/", "dir/symdir -> realdir", "dir/realdir/file"] self.addpkg2db("local", lp) p = pmpkg("pkg", "1.0-2") p.files = ["dir/symdir/file"] self.addpkg(p) self.args = "-U %s" % p.filename() self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_EXIST=pkg") self.addrule("PKG_VERSION=pkg|1.0-2") +self.addrule("FILE_TYPE=dir/symdir/|dir") diff --git a/test/pacman/tests/fileconflict013.py b/test/pacman/tests/fileconflict013.py index a83923c..5c3a43b 100644 --- a/test/pacman/tests/fileconflict013.py +++ b/test/pacman/tests/fileconflict013.py @@ -1,20 +1,23 @@ self.description = "file->file path change with same effective path (/lib as symlink)" +# Note: this situation means the filesystem and local db are out of sync lp1 = pmpkg("filesystem", "1.0-1") lp1.files = ["usr/", "usr/lib/", "lib -> usr/lib/"] self.addpkg2db("local", lp1) lp2 = pmpkg("pkg1", "1.0-1") lp2.files = ["lib/libfoo.so"] self.addpkg2db("local", lp2) sp1 = pmpkg("pkg1", "1.0-2") sp1.files = ["usr/lib/libfoo.so"] self.addpkg2db("sync", sp1) self.args = "-Su" -self.addrule("PACMAN_RETCODE=0") -self.addrule("PKG_VERSION=pkg1|1.0-2") +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_VERSION=pkg1|1.0-1") + +self.expectfailure = True diff --git a/test/pacman/tests/fileconflict022.py b/test/pacman/tests/fileconflict022.py index 5759d4c..5ff0b53 100644 --- a/test/pacman/tests/fileconflict022.py +++ b/test/pacman/tests/fileconflict022.py @@ -1,19 +1,18 @@ self.description = "File conflict between package with symlink and package with real path" self.filesystem = ["usr/lib/", "lib -> usr/lib/"] sp1 = pmpkg("foo") # share/ causes the entries to be reordered after path resolution sp1.files = ["lib/", "lib/file", "share/"] self.addpkg2db("sync", sp1) sp2 = pmpkg("bar") sp2.files = ["usr/", "usr/lib/", "usr/lib/file"] self.addpkg2db("sync", sp2) self.args = "-S %s %s" % (sp1.name, sp2.name) self.addrule("PACMAN_RETCODE=1") -self.addrule("PACMAN_OUTPUT=.*/usr/lib/file exists in both 'foo' and 'bar'") self.addrule("!PKG_EXIST=foo") self.addrule("!PKG_EXIST=bar") diff --git a/test/pacman/tests/fileconflict023.py b/test/pacman/tests/fileconflict023.py index ce72087..9685c0d 100644 --- a/test/pacman/tests/fileconflict023.py +++ b/test/pacman/tests/fileconflict023.py @@ -1,18 +1,21 @@ self.description = "File conflict between package with symlink and package with real path resolved by removal" +# Note: this situation means the filesystem and local db are out of sync self.filesystem = ["usr/", "usr/lib/", "lib -> usr/lib/"] lp1 = pmpkg("foo") lp1.files = ["lib/", "lib/file"] self.addpkg2db("local", lp1) sp1 = pmpkg("bar") sp1.conflicts = ["foo"] sp1.files = ["usr/", "usr/lib/", "usr/lib/file"] self.addpkg2db("sync", sp1) self.args = "-S %s --ask=4" % sp1.name -self.addrule("PACMAN_RETCODE=0") -self.addrule("!PKG_EXIST=foo") -self.addrule("PKG_EXIST=bar") +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_EXIST=foo") +self.addrule("!PKG_EXIST=bar") + +self.expectfailure = True diff --git a/test/pacman/tests/fileconflict025.py b/test/pacman/tests/fileconflict025.py index 3c6f063..195badb 100644 --- a/test/pacman/tests/fileconflict025.py +++ b/test/pacman/tests/fileconflict025.py @@ -1,18 +1,20 @@ -self.description = "File conflict between package with symlink and package with real path resolved by removal (reversed)" +self.description = "File conflict between package with symlink and package with real path and filesystem (reversed)" self.filesystem = ["usr/lib/", "lib -> usr/lib/"] lp1 = pmpkg("foo") lp1.files = ["usr/", "usr/lib/", "usr/lib/file"] self.addpkg2db("local", lp1) sp1 = pmpkg("bar") sp1.conflicts = ["foo"] sp1.files = ["lib/", "lib/file"] self.addpkg2db("sync", sp1) self.args = "-S %s --ask=4" % sp1.name -self.addrule("PACMAN_RETCODE=0") -self.addrule("!PKG_EXIST=foo") -self.addrule("PKG_EXIST=bar") +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_EXIST=foo") +self.addrule("!PKG_EXIST=bar") + +self.expectfailure = True diff --git a/test/pacman/tests/symlink001.py b/test/pacman/tests/symlink001.py index cbf71cc..f88d0ae 100644 --- a/test/pacman/tests/symlink001.py +++ b/test/pacman/tests/symlink001.py @@ -1,20 +1,21 @@ self.description = "Dir symlinks overwritten on install (the perl/git bug)" lp = pmpkg("dummy") lp.files = ["dir/realdir/", "dir/symdir -> realdir"] self.addpkg2db("local", lp) p = pmpkg("pkg1") p.files = ["dir/symdir/tmp"] self.addpkg(p) self.args = "-U %s" % p.filename() -self.addrule("PACMAN_RETCODE=0") -self.addrule("PKG_EXIST=pkg1") -self.addrule("FILE_EXIST=dir/symdir/tmp") -self.addrule("FILE_EXIST=dir/realdir/tmp") -self.addrule("FILE_TYPE=dir/symdir/tmp|file") +self.addrule("PACMAN_RETCODE=1") +self.addrule("!PKG_EXIST=pkg1") +self.addrule("!FILE_EXIST=dir/symdir/tmp") +self.addrule("!FILE_EXIST=dir/realdir/tmp") self.addrule("FILE_TYPE=dir/symdir|link") self.addrule("FILE_TYPE=dir/realdir|dir") + +self.expectfailure = True diff --git a/test/pacman/tests/sync700.py b/test/pacman/tests/sync700.py index 9748c81..3948b00 100644 --- a/test/pacman/tests/sync700.py +++ b/test/pacman/tests/sync700.py @@ -1,22 +1,22 @@ -self.description = "do not remove directory symlink if another package has file in its path" +self.description = "removal of directory symlink when another package has file in its path" +# Note: this situation means that the filesystem and local db are out of sync lp1 = pmpkg("pkg1") lp1.files = ["usr/lib/foo", "lib -> usr/lib"] self.addpkg2db("local", lp1) lp2 = pmpkg("pkg2") lp2.files = ["lib/bar"] self.addpkg2db("local", lp2) p = pmpkg("pkg1", "1.0-2") p.files = ["usr/lib/foo"] self.addpkg2db("sync", p) self.args = "-S pkg1" -self.addrule("PACMAN_RETCODE=1") -self.addrule("PKG_VERSION=pkg1|1.0-1") -self.addrule("FILE_EXIST=lib/bar") - -self.expectfailure = True +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=pkg1|1.0-2") +self.addrule("FILE_EXIST=usr/lib/bar") +self.addrule("!FILE_EXIST=lib/bar") diff --git a/test/pacman/tests/sync701.py b/test/pacman/tests/sync701.py index 201f602..590845f 100644 --- a/test/pacman/tests/sync701.py +++ b/test/pacman/tests/sync701.py @@ -1,22 +1,21 @@ -self.description = "do not remove directory symlink if incoming package has file in its path (order 1)" +self.description = "incoming package replaces symlink with directory (order 1)" lp = pmpkg("pkg1") lp.files = ["usr/lib/foo", "lib -> usr/lib"] self.addpkg2db("local", lp) p1 = pmpkg("pkg1", "1.0-2") p1.files = ["usr/lib/foo"] self.addpkg2db("sync", p1) p2 = pmpkg("pkg2") p2.files = ["lib/bar"] self.addpkg2db("sync", p2) self.args = "-S pkg1 pkg2" -self.addrule("PACMAN_RETCODE=1") -self.addrule("PKG_VERSION=pkg1|1.0-1") -self.addrule("!PKG_EXIST=pkg2") - -self.expectfailure = True +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=pkg1|1.0-2") +self.addrule("PKG_EXIST=pkg2") +self.addrule("FILE_TYPE=lib|dir") diff --git a/test/pacman/tests/sync702.py b/test/pacman/tests/sync702.py index ee4eef9..8f4c0ad 100644 --- a/test/pacman/tests/sync702.py +++ b/test/pacman/tests/sync702.py @@ -1,22 +1,23 @@ -self.description = "do not remove directory symlink if incoming package has file in its path (order 2)" +self.description = "incoming package replaces symlink with directory (order 2)" lp = pmpkg("pkg2") lp.files = ["usr/lib/foo", "lib -> usr/lib"] self.addpkg2db("local", lp) p1 = pmpkg("pkg1") p1.files = ["lib/bar"] self.addpkg2db("sync", p1) p2 = pmpkg("pkg2", "1.0-2") p2.files = ["usr/lib/foo"] self.addpkg2db("sync", p2) self.args = "-S pkg1 pkg2" -self.addrule("PACMAN_RETCODE=1") -self.addrule("PKG_VERSION=pkg2|1.0-1") -self.addrule("!PKG_EXIST=pkg1") +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=pkg2|1.0-2") +self.addrule("PKG_EXIST=pkg1") +self.addrule("FILE_TYPE=lib|dir") self.expectfailure = True -- 1.8.2.1
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/alpm.h | 3 +- lib/libalpm/conflict.c | 14 +-- lib/libalpm/filelist.c | 225 ++--------------------------------- lib/libalpm/filelist.h | 4 - lib/libalpm/package.c | 22 +--- lib/libalpm/remove.c | 5 - test/pacman/tests/fileconflict001.py | 2 + test/pacman/tests/fileconflict013.py | 2 - test/pacman/tests/fileconflict016.py | 2 + test/pacman/tests/fileconflict017.py | 2 + test/pacman/tests/fileconflict022.py | 2 + test/pacman/tests/fileconflict023.py | 2 - test/pacman/tests/fileconflict025.py | 2 - 13 files changed, 24 insertions(+), 263 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index ccbdd1c..2cdac0c 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -217,7 +217,6 @@ typedef struct _alpm_file_t { typedef struct _alpm_filelist_t { size_t count; alpm_file_t *files; - char **resolved_path; } alpm_filelist_t; /** Local package or package file backup entry */ @@ -1043,7 +1042,7 @@ int alpm_pkg_set_reason(alpm_pkg_t *pkg, alpm_pkgreason_t reason); * @param path the path to search for in the package * @return a pointer to the matching file or NULL if not found */ -char *alpm_filelist_contains(alpm_filelist_t *filelist, const char *path); +alpm_file_t *alpm_filelist_contains(alpm_filelist_t *filelist, const char *path); /* * Signatures diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 18e29a8..5e2fa1e 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -321,7 +321,6 @@ static int dir_belongsto_pkg(alpm_handle_t *handle, const char *dirpath, const char *root = handle->root; /* check directory is actually in package - used for subdirectory checks */ - _alpm_filelist_resolve(handle, alpm_pkg_get_files(pkg)); if(!alpm_filelist_contains(alpm_pkg_get_files(pkg), dirpath)) { _alpm_log(handle, ALPM_LOG_DEBUG, "directory %s not in package %s\n", dirpath, pkg->name); @@ -340,7 +339,6 @@ static int dir_belongsto_pkg(alpm_handle_t *handle, const char *dirpath, continue; } - _alpm_filelist_resolve(handle, alpm_pkg_get_files(i->data)); if(alpm_filelist_contains(alpm_pkg_get_files(i->data), dirpath)) { _alpm_log(handle, ALPM_LOG_DEBUG, "file %s also in package %s\n", dirpath, @@ -375,7 +373,6 @@ static int dir_belongsto_pkg(alpm_handle_t *handle, const char *dirpath, return 0; } } else { - _alpm_filelist_resolve(handle, alpm_pkg_get_files(pkg)); if(alpm_filelist_contains(alpm_pkg_get_files(pkg), path)) { continue; } else { @@ -417,11 +414,6 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, rootlen = strlen(handle->root); - /* make sure all files to be installed have been resolved */ - for(i = upgrade; i; i = i->next) { - _alpm_filelist_resolve(handle, alpm_pkg_get_files(i->data)); - } - /* TODO this whole function needs a huge change, which hopefully will * be possible with real transactions. Right now we only do half as much * here as we do when we actually extract files in add.c with our 12 @@ -491,7 +483,6 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, * be freed. */ if(dbpkg) { /* older ver of package currently installed */ - _alpm_filelist_resolve(handle, alpm_pkg_get_files(dbpkg)); tmpfiles = _alpm_filelist_difference(alpm_pkg_get_files(p1), alpm_pkg_get_files(dbpkg)); } else { @@ -499,7 +490,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, alpm_filelist_t *fl = alpm_pkg_get_files(p1); size_t filenum; for(filenum = 0; filenum < fl->count; filenum++) { - tmpfiles = alpm_list_add(tmpfiles, fl->resolved_path[filenum]); + tmpfiles = alpm_list_add(tmpfiles, fl->files[filenum].name); } } @@ -545,7 +536,6 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, /* Check remove list (will we remove the conflicting local file?) */ for(k = rem; k && !resolved_conflict; k = k->next) { alpm_pkg_t *rempkg = k->data; - _alpm_filelist_resolve(handle, alpm_pkg_get_files(rempkg)); if(rempkg && alpm_filelist_contains(alpm_pkg_get_files(rempkg), relative_path)) { _alpm_log(handle, ALPM_LOG_DEBUG, @@ -563,7 +553,6 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, alpm_pkg_t *localp2 = _alpm_db_get_pkgfromcache(handle->db_local, p2->name); /* localp2->files will be removed (target conflicts are handled by CHECK 1) */ - _alpm_filelist_resolve(handle, alpm_pkg_get_files(localp2)); if(localp2 && alpm_filelist_contains(alpm_pkg_get_files(localp2), filestr)) { /* skip removal of file, but not add. this will prevent a second * package from removing the file when it was already installed @@ -610,7 +599,6 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, alpm_list_t *local_pkgs = _alpm_db_get_pkgcache(handle->db_local); int found = 0; for(k = local_pkgs; k && !found; k = k->next) { - _alpm_filelist_resolve(handle, alpm_pkg_get_files(k->data)); if(alpm_filelist_contains(alpm_pkg_get_files(k->data), filestr)) { found = 1; } diff --git a/lib/libalpm/filelist.c b/lib/libalpm/filelist.c index 697dd23..f8db9a3 100644 --- a/lib/libalpm/filelist.c +++ b/lib/libalpm/filelist.c @@ -25,199 +25,6 @@ #include "filelist.h" #include "util.h" -/** Helper function for comparing strings when sorting */ -static int _alpm_filelist_strcmp(const void *s1, const void *s2) -{ - return strcmp(*(char **)s1, *(char **)s2); -} - -/* TODO make sure callers check the return value so we can bail on errors. - * For now we soldier on as best we can, skipping paths that are too long to - * resolve and using the original filenames on memory errors. */ -/** - * @brief Resolves a symlink and its children. - * - * @attention Pre-condition: files must be sorted! - * - * @param files filelist to resolve - * @param i pointer to the index in files to start processing, will point to - * the last file processed on return - * @param path absolute path for the symlink being resolved - * @param root_len length of the root portion of path - * @param resolving is file \i in \files a symlink that needs to be resolved - * - * @return 0 on success, -1 on error - */ -int _alpm_filelist_resolve_link(alpm_filelist_t *files, size_t *i, - char *path, size_t root_len, int resolving) -{ - char *causal_dir = NULL; /* symlink being resolved */ - char *filename_r = NULL; /* resolved filename */ - size_t causal_dir_len = 0, causal_dir_r_len = 0; - - if(resolving) { - /* deal with the symlink being resolved */ - MALLOC(filename_r, PATH_MAX, goto error); - causal_dir = files->files[*i].name; - causal_dir_len = strlen(causal_dir); - if(realpath(path, filename_r) == NULL) { - files->resolved_path[*i] = causal_dir; - FREE(filename_r); - return -1; - } - causal_dir_r_len = strlen(filename_r + root_len) + 1; - if(causal_dir_r_len >= PATH_MAX) { - files->resolved_path[*i] = causal_dir; - FREE(filename_r); - return -1; - } - /* remove root_r from filename_r */ - memmove(filename_r, filename_r + root_len, causal_dir_r_len); - filename_r[causal_dir_r_len - 1] = '/'; - filename_r[causal_dir_r_len] = '\0'; - STRDUP(files->resolved_path[*i], filename_r, goto error); - (*i)++; - } - - for(; *i < files->count; (*i)++) { - char *filename = files->files[*i].name; - size_t filename_len = strlen(filename); - size_t filename_r_len = filename_len; - struct stat sbuf; - int exists; - - if(resolving) { - if(filename_len < causal_dir_len || strncmp(filename, causal_dir, causal_dir_len) != 0) { - /* not inside causal_dir anymore */ - break; - } - - filename_r_len = filename_len + causal_dir_r_len - causal_dir_len; - if(filename_r_len >= PATH_MAX) { - /* resolved path is too long */ - files->resolved_path[*i] = filename; - continue; - } - - strcpy(filename_r + causal_dir_r_len, filename + causal_dir_len); - } - - /* deal with files and paths too long to resolve*/ - if(filename[filename_len - 1] != '/' || root_len + filename_r_len >= PATH_MAX) { - if(resolving) { - STRDUP(files->resolved_path[*i], filename_r, goto error); - } else { - files->resolved_path[*i] = filename; - } - continue; - } - - /* construct absolute path and stat() */ - strcpy(path + root_len, resolving ? filename_r : filename); - exists = !_alpm_lstat(path, &sbuf); - - /* deal with symlinks */ - if(exists && S_ISLNK(sbuf.st_mode)) { - _alpm_filelist_resolve_link(files, i, path, root_len, 1); - continue; - } - - /* deal with normal directories */ - if(resolving) { - STRDUP(files->resolved_path[*i], filename_r, goto error); - } else { - files->resolved_path[*i] = filename; - } - - /* deal with children of non-existent directories to reduce lstat() calls */ - if(!exists) { - for((*i)++; *i < files->count; (*i)++) { - char *f = files->files[*i].name; - size_t f_len = strlen(f); - size_t f_r_len; - - if(f_len < filename_len || strncmp(f, filename, filename_len) != 0) { - /* not inside the non-existent dir anymore */ - break; - } - - f_r_len = f_len + causal_dir_r_len - causal_dir_len; - if(resolving && f_r_len <= PATH_MAX) { - strcpy(filename_r + causal_dir_r_len, f + causal_dir_len); - STRDUP(files->resolved_path[*i], filename_r, goto error); - } else { - files->resolved_path[*i] = f; - } - } - (*i)--; - } - } - (*i)--; - - FREE(filename_r); - - return 0; - -error: - FREE(filename_r); - /* out of memory, set remaining files to their original names */ - for(; *i < files->count; (*i)++) { - files->resolved_path[*i] = files->files[*i].name; - } - (*i)--; - return -1; -} - -/** - * @brief Takes a file list and resolves all directory paths according to the - * filesystem - * - * @attention Pre-condition: files must be sorted! - * - * @note A symlink and directory at the same path in two difference packages - * causes a conflict so the filepath can not change as packages get installed - * - * @param handle the context handle - * @param files list of files to resolve - * - * @return 0 on success, -1 on error - */ -int _alpm_filelist_resolve(alpm_handle_t *handle, alpm_filelist_t *files) -{ - char path[PATH_MAX]; - size_t root_len, i = 0; - int ret = 0; - - if(!files || files->resolved_path) { - return 0; - } - - CALLOC(files->resolved_path, files->count, sizeof(char *), return -1); - - /* not much point in going on if we can't even resolve root */ - if(realpath(handle->root, path) == NULL){ - return -1; - } - root_len = strlen(path); - if(root_len + 1 >= PATH_MAX) { - return -1; - } - /* append '/' if root is not "/" */ - if(path[root_len - 1] != '/') { - path[root_len] = '/'; - root_len++; - path[root_len] = '\0'; - } - - ret = _alpm_filelist_resolve_link(files, &i, path, root_len, 0); - - qsort(files->resolved_path, files->count, sizeof(char *), - _alpm_filelist_strcmp); - - return ret; -} - - /* Returns the difference of the provided two lists of files. * Pre-condition: both lists are sorted! * When done, free the list but NOT the contained data. @@ -229,8 +36,8 @@ alpm_list_t *_alpm_filelist_difference(alpm_filelist_t *filesA, size_t ctrA = 0, ctrB = 0; while(ctrA < filesA->count && ctrB < filesB->count) { - char *strA = filesA->resolved_path[ctrA]; - char *strB = filesB->resolved_path[ctrB]; + char *strA = filesA->files[ctrA].name; + char *strB = filesB->files[ctrB].name; int cmp = strcmp(strA, strB); if(cmp < 0) { @@ -247,7 +54,7 @@ alpm_list_t *_alpm_filelist_difference(alpm_filelist_t *filesA, /* ensure we have completely emptied pA */ while(ctrA < filesA->count) { - ret = alpm_list_add(ret, filesA->resolved_path[ctrA]); + ret = alpm_list_add(ret, filesA->files[ctrA].name); ctrA++; } @@ -269,17 +76,17 @@ alpm_list_t *_alpm_filelist_intersection(alpm_filelist_t *filesA, char *strA, *strB; isdirA = 0; - strA = filesA->resolved_path[ctrA]; + strA = filesA->files[ctrA].name; if(strA[strlen(strA)-1] == '/') { isdirA = 1; - strA = strndup(filesA->resolved_path[ctrA], strlen(strA)-1); + strA = strndup(strA, strlen(strA)-1); } isdirB = 0; - strB = filesB->resolved_path[ctrB]; + strB = filesB->files[ctrB].name; if(strB[strlen(strB)-1] == '/') { isdirB = 1; - strB = strndup(filesB->resolved_path[ctrB], strlen(strB)-1); + strB = strndup(strB, strlen(strB)-1); } cmp = strcmp(strA, strB); @@ -297,7 +104,7 @@ alpm_list_t *_alpm_filelist_intersection(alpm_filelist_t *filesA, /* when not directories, item in both qualifies as an intersect */ if(! (isdirA && isdirB)) { - ret = alpm_list_add(ret, filesA->resolved_path[ctrA]); + ret = alpm_list_add(ret, filesA->files[ctrA].name); } ctrA++; ctrB++; @@ -323,11 +130,10 @@ int _alpm_files_cmp(const void *f1, const void *f2) return strcmp(file1->name, file2->name); } - -char SYMEXPORT *alpm_filelist_contains(alpm_filelist_t *filelist, +alpm_file_t SYMEXPORT *alpm_filelist_contains(alpm_filelist_t *filelist, const char *path) { - alpm_file_t key, *match; + alpm_file_t key; if(!filelist) { return NULL; @@ -335,17 +141,8 @@ char SYMEXPORT *alpm_filelist_contains(alpm_filelist_t *filelist, key.name = (char *)path; - match = bsearch(&key, filelist->files, filelist->count, + return bsearch(&key, filelist->files, filelist->count, sizeof(alpm_file_t), _alpm_files_cmp); - - if(match) { - return match->name; - } else if(filelist->resolved_path) { - return bsearch(&path, filelist->resolved_path, filelist->count, - sizeof(char *), _alpm_filelist_strcmp); - } else { - return NULL; - } } /* vim: set ts=2 sw=2 noet: */ diff --git a/lib/libalpm/filelist.h b/lib/libalpm/filelist.h index bfab416..66501f3 100644 --- a/lib/libalpm/filelist.h +++ b/lib/libalpm/filelist.h @@ -21,10 +21,6 @@ #include "alpm.h" -int _alpm_filelist_resolve_link(alpm_filelist_t *files, size_t *i, - char *path, size_t root_len, int resolving); -int _alpm_filelist_resolve(alpm_handle_t *handle, alpm_filelist_t *files); - alpm_list_t *_alpm_filelist_difference(alpm_filelist_t *filesA, alpm_filelist_t *filesB); diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c index 098c867..cfdbb3f 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -606,9 +606,6 @@ int _alpm_pkg_dup(alpm_pkg_t *pkg, alpm_pkg_t **new_ptr) } } newpkg->files.count = pkg->files.count; - /* deliberately do not copy resolved_path as this is only used - * during conflict checking and the sorting of list does not readily - * allow keeping its efficient memory usage when copying */ } /* internal */ @@ -657,22 +654,9 @@ void _alpm_pkg_free(alpm_pkg_t *pkg) free_deplist(pkg->replaces); FREELIST(pkg->groups); if(pkg->files.count) { - size_t i, j, k; - if(pkg->files.resolved_path) { - for(i = 0, j = 0; i < pkg->files.count; i++) { - for(k = j; k <= pkg->files.count; k++) { - if(pkg->files.resolved_path[i] == pkg->files.files[k].name) { - pkg->files.files[k].name = NULL; - j = k + 1; - break; - } - } - free(pkg->files.resolved_path[i]); - } - free(pkg->files.resolved_path); - } - for(j = 0; j < pkg->files.count; j++) { - FREE(pkg->files.files[j].name); + size_t i; + for(i = 0; i < pkg->files.count; i++) { + FREE(pkg->files.files[i].name); } free(pkg->files.files); } diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index d0cd613..652aa50 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -462,7 +462,6 @@ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg, "keeping directory %s (could not count files)\n", file); } else if(newpkg && alpm_filelist_contains(alpm_pkg_get_files(newpkg), fileobj->name)) { - /* newpkg's filelist should have already been resolved by the caller */ _alpm_log(handle, ALPM_LOG_DEBUG, "keeping directory %s (in new package)\n", file); } else if(dir_is_mountpoint(handle, file, &buf)) { @@ -484,9 +483,6 @@ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg, continue; } filelist = alpm_pkg_get_files(local_pkg); - /* This is too slow and only covers a rare case - Disable for now... */ - /* _alpm_filelist_resolve(handle, filelist); */ if(alpm_filelist_contains(filelist, fileobj->name)) { _alpm_log(handle, ALPM_LOG_DEBUG, "keeping directory %s (owned by %s)\n", file, local_pkg->name); @@ -584,7 +580,6 @@ static int remove_package_files(alpm_handle_t *handle, * so this removal operation doesn't kill them */ /* old package backup list */ newfiles = alpm_pkg_get_files(newpkg); - _alpm_filelist_resolve(handle, newfiles); for(b = alpm_pkg_get_backup(newpkg); b; b = b->next) { const alpm_backup_t *backup = b->data; /* safety check (fix the upgrade026 pactest) */ diff --git a/test/pacman/tests/fileconflict001.py b/test/pacman/tests/fileconflict001.py index b1ad5e1..e1371f8 100644 --- a/test/pacman/tests/fileconflict001.py +++ b/test/pacman/tests/fileconflict001.py @@ -25,3 +25,5 @@ self.addrule("!PKG_EXIST=pkg2") self.addrule("FILE_EXIST=dir/realdir/realfile") self.addrule("!FILE_EXIST=dir/realdir/file") + +self.expectfailure = True diff --git a/test/pacman/tests/fileconflict013.py b/test/pacman/tests/fileconflict013.py index 5c3a43b..aacb730 100644 --- a/test/pacman/tests/fileconflict013.py +++ b/test/pacman/tests/fileconflict013.py @@ -19,5 +19,3 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("PKG_VERSION=pkg1|1.0-1") - -self.expectfailure = True diff --git a/test/pacman/tests/fileconflict016.py b/test/pacman/tests/fileconflict016.py index 86ddd72..dbcf708 100644 --- a/test/pacman/tests/fileconflict016.py +++ b/test/pacman/tests/fileconflict016.py @@ -22,3 +22,5 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("!PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") + +self.expectfailure = True diff --git a/test/pacman/tests/fileconflict017.py b/test/pacman/tests/fileconflict017.py index 3855a93..4b91dc6 100644 --- a/test/pacman/tests/fileconflict017.py +++ b/test/pacman/tests/fileconflict017.py @@ -24,3 +24,5 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("!PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") + +self.expectfailure = True diff --git a/test/pacman/tests/fileconflict022.py b/test/pacman/tests/fileconflict022.py index 5ff0b53..b3920df 100644 --- a/test/pacman/tests/fileconflict022.py +++ b/test/pacman/tests/fileconflict022.py @@ -16,3 +16,5 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("!PKG_EXIST=foo") self.addrule("!PKG_EXIST=bar") + +self.expectfailure = True diff --git a/test/pacman/tests/fileconflict023.py b/test/pacman/tests/fileconflict023.py index 9685c0d..1a7eaaf 100644 --- a/test/pacman/tests/fileconflict023.py +++ b/test/pacman/tests/fileconflict023.py @@ -17,5 +17,3 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("PKG_EXIST=foo") self.addrule("!PKG_EXIST=bar") - -self.expectfailure = True diff --git a/test/pacman/tests/fileconflict025.py b/test/pacman/tests/fileconflict025.py index 195badb..263c81d 100644 --- a/test/pacman/tests/fileconflict025.py +++ b/test/pacman/tests/fileconflict025.py @@ -16,5 +16,3 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("PKG_EXIST=foo") self.addrule("!PKG_EXIST=bar") - -self.expectfailure = True -- 1.8.2.1
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/conflict.c | 7 ------- test/pacman/tests/fileconflict001.py | 2 -- test/pacman/tests/fileconflict007.py | 2 ++ test/pacman/tests/fileconflict016.py | 2 -- test/pacman/tests/fileconflict017.py | 2 -- test/pacman/tests/fileconflict022.py | 2 -- test/pacman/tests/symlink001.py | 2 -- test/pacman/tests/sync701.py | 2 ++ 8 files changed, 4 insertions(+), 17 deletions(-) diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 5e2fa1e..518a2c6 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -514,17 +514,10 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, _alpm_log(handle, ALPM_LOG_DEBUG, "checking possible conflict: %s\n", path); if(filestr[strlen(filestr) - 1] == '/') { - struct stat sbuf; if(S_ISDIR(lsbuf.st_mode)) { _alpm_log(handle, ALPM_LOG_DEBUG, "file is a directory, not a conflict\n"); continue; } - stat(path, &sbuf); - if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(sbuf.st_mode)) { - _alpm_log(handle, ALPM_LOG_DEBUG, - "file is a symlink to a dir, hopefully not a conflict\n"); - continue; - } /* if we made it to here, we want all subsequent path comparisons to * not include the trailing slash. This allows things like file -> * directory replacements. */ diff --git a/test/pacman/tests/fileconflict001.py b/test/pacman/tests/fileconflict001.py index e1371f8..b1ad5e1 100644 --- a/test/pacman/tests/fileconflict001.py +++ b/test/pacman/tests/fileconflict001.py @@ -25,5 +25,3 @@ self.addrule("!PKG_EXIST=pkg2") self.addrule("FILE_EXIST=dir/realdir/realfile") self.addrule("!FILE_EXIST=dir/realdir/file") - -self.expectfailure = True diff --git a/test/pacman/tests/fileconflict007.py b/test/pacman/tests/fileconflict007.py index 7fe65ed..b61ddb4 100644 --- a/test/pacman/tests/fileconflict007.py +++ b/test/pacman/tests/fileconflict007.py @@ -16,3 +16,5 @@ self.addrule("PKG_EXIST=pkg") self.addrule("PKG_VERSION=pkg|1.0-2") self.addrule("FILE_TYPE=dir/symdir/|dir") + +self.expectfailure = True diff --git a/test/pacman/tests/fileconflict016.py b/test/pacman/tests/fileconflict016.py index dbcf708..86ddd72 100644 --- a/test/pacman/tests/fileconflict016.py +++ b/test/pacman/tests/fileconflict016.py @@ -22,5 +22,3 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("!PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") - -self.expectfailure = True diff --git a/test/pacman/tests/fileconflict017.py b/test/pacman/tests/fileconflict017.py index 4b91dc6..3855a93 100644 --- a/test/pacman/tests/fileconflict017.py +++ b/test/pacman/tests/fileconflict017.py @@ -24,5 +24,3 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("!PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") - -self.expectfailure = True diff --git a/test/pacman/tests/fileconflict022.py b/test/pacman/tests/fileconflict022.py index b3920df..5ff0b53 100644 --- a/test/pacman/tests/fileconflict022.py +++ b/test/pacman/tests/fileconflict022.py @@ -16,5 +16,3 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("!PKG_EXIST=foo") self.addrule("!PKG_EXIST=bar") - -self.expectfailure = True diff --git a/test/pacman/tests/symlink001.py b/test/pacman/tests/symlink001.py index f88d0ae..9136551 100644 --- a/test/pacman/tests/symlink001.py +++ b/test/pacman/tests/symlink001.py @@ -17,5 +17,3 @@ self.addrule("!FILE_EXIST=dir/realdir/tmp") self.addrule("FILE_TYPE=dir/symdir|link") self.addrule("FILE_TYPE=dir/realdir|dir") - -self.expectfailure = True diff --git a/test/pacman/tests/sync701.py b/test/pacman/tests/sync701.py index 590845f..912c794 100644 --- a/test/pacman/tests/sync701.py +++ b/test/pacman/tests/sync701.py @@ -19,3 +19,5 @@ self.addrule("PKG_VERSION=pkg1|1.0-2") self.addrule("PKG_EXIST=pkg2") self.addrule("FILE_TYPE=lib|dir") + +self.expectfailure = True -- 1.8.2.1
In the event of a symlink<->dir transition, the paths for the incoming package need to be resolved to match the filelist for installed packages. Note: this only enables the symlink<->dir transition itself. Any installed packages with invalid file lists containing symlinks are not detected. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/conflict.c | 42 ++++++++++++++++------------------- src/common/util-common.c | 43 ++++++++++++++++++++++++++++++++++++ src/common/util-common.h | 1 + test/pacman/tests/fileconflict007.py | 2 -- test/pacman/tests/sync701.py | 2 -- test/pacman/tests/sync702.py | 2 -- 6 files changed, 63 insertions(+), 29 deletions(-) diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 518a2c6..08c1cbe 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -501,7 +501,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, /* have we acted on this conflict? */ int resolved_conflict = 0; struct stat lsbuf; - char path[PATH_MAX]; + char path[PATH_MAX], resolved_path[PATH_MAX]; size_t pathlen; pathlen = snprintf(path, PATH_MAX, "%s%s", handle->root, filestr); @@ -513,7 +513,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, _alpm_log(handle, ALPM_LOG_DEBUG, "checking possible conflict: %s\n", path); - if(filestr[strlen(filestr) - 1] == '/') { + if(path[pathlen - 1] == '/') { if(S_ISDIR(lsbuf.st_mode)) { _alpm_log(handle, ALPM_LOG_DEBUG, "file is a directory, not a conflict\n"); continue; @@ -524,7 +524,19 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, path[pathlen - 1] = '\0'; } - relative_path = path + rootlen; + if(lrealpath(path, resolved_path) + && strncmp(resolved_path, handle->root, rootlen) == 0) { + relative_path = resolved_path + rootlen; + } else { + relative_path = path + rootlen; + } + + if(!resolved_conflict && dbpkg + && alpm_filelist_contains(alpm_pkg_get_files(dbpkg), relative_path)) { + _alpm_log(handle, ALPM_LOG_DEBUG, + "package contained the resolved realpath\n"); + resolved_conflict = 1; + } /* Check remove list (will we remove the conflicting local file?) */ for(k = rem; k && !resolved_conflict; k = k->next) { @@ -546,12 +558,12 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, alpm_pkg_t *localp2 = _alpm_db_get_pkgfromcache(handle->db_local, p2->name); /* localp2->files will be removed (target conflicts are handled by CHECK 1) */ - if(localp2 && alpm_filelist_contains(alpm_pkg_get_files(localp2), filestr)) { + if(localp2 && alpm_filelist_contains(alpm_pkg_get_files(localp2), relative_path)) { /* skip removal of file, but not add. this will prevent a second * package from removing the file when it was already installed * by its new owner (whether the file is in backup array or not */ handle->trans->skip_remove = - alpm_list_add(handle->trans->skip_remove, strdup(filestr)); + alpm_list_add(handle->trans->skip_remove, strdup(relative_path)); _alpm_log(handle, ALPM_LOG_DEBUG, "file changed packages, adding to remove skiplist\n"); resolved_conflict = 1; @@ -560,8 +572,8 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, /* check if all files of the dir belong to the installed pkg */ if(!resolved_conflict && S_ISDIR(lsbuf.st_mode) && dbpkg) { - char *dir = malloc(strlen(filestr) + 2); - sprintf(dir, "%s/", filestr); + char *dir = malloc(strlen(relative_path) + 2); + sprintf(dir, "%s/", relative_path); if(alpm_filelist_contains(alpm_pkg_get_files(dbpkg), dir)) { _alpm_log(handle, ALPM_LOG_DEBUG, "checking if all files in %s belong to %s\n", @@ -571,22 +583,6 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, free(dir); } - /* check if a component of the filepath was a link. canonicalize the path - * and look for it in the old package. note that the actual file under - * consideration cannot itself be a link, as it might be unowned- path - * components can be safely checked as all directories are "unowned". */ - if(!resolved_conflict && dbpkg && !S_ISLNK(lsbuf.st_mode)) { - char rpath[PATH_MAX]; - if(realpath(path, rpath)) { - const char *relative_rpath = rpath + rootlen; - if(alpm_filelist_contains(alpm_pkg_get_files(dbpkg), relative_rpath)) { - _alpm_log(handle, ALPM_LOG_DEBUG, - "package contained the resolved realpath\n"); - resolved_conflict = 1; - } - } - } - /* is the file unowned and in the backup list of the new package? */ if(!resolved_conflict && _alpm_needbackup(filestr, p1)) { alpm_list_t *local_pkgs = _alpm_db_get_pkgcache(handle->db_local); diff --git a/src/common/util-common.c b/src/common/util-common.c index 7145e49..c2422e0 100644 --- a/src/common/util-common.c +++ b/src/common/util-common.c @@ -73,6 +73,49 @@ char *mdirname(const char *path) return strdup("."); } +/** Resolve the canonicalized absolute path of a symlink. + * @param path path to resolve + * @param resolved_path destination for the resolved path, will be malloc'd if + * NULL + * @return the resolved path + */ +char *lrealpath(const char *path, char *resolved_path) +{ + const char *bname = mbasename(path); + char *rpath = NULL, *dname = NULL; + int success = 0; + + if(strcmp(path, ".") == 0 || strcmp(path, "..") == 0) { + /* the entire path needs to be resolved */ + return realpath(path, resolved_path); + } + + if(!(dname = mdirname(path))) { + goto cleanup; + } + if(!(rpath = realpath(dname, NULL))) { + goto cleanup; + } + if(!resolved_path) { + if(!(resolved_path = malloc(strlen(rpath) + strlen(bname) + 2))) { + goto cleanup; + } + } + + strcpy(resolved_path, rpath); + if(resolved_path[strlen(resolved_path) - 1] != '/') { + strcat(resolved_path, "/"); + } + strcat(resolved_path, bname); + success = 1; + +cleanup: + free(dname); + free(rpath); + + return (success ? resolved_path : NULL); +} + #ifndef HAVE_STRNDUP /* A quick and dirty implementation derived from glibc */ /** Determines the length of a fixed-size string. diff --git a/src/common/util-common.h b/src/common/util-common.h index 04d4e9d..b3a4df1 100644 --- a/src/common/util-common.h +++ b/src/common/util-common.h @@ -22,6 +22,7 @@ const char *mbasename(const char *path); char *mdirname(const char *path); +char *lrealpath(const char *path, char *resolved_path); #ifndef HAVE_STRNDUP char *strndup(const char *s, size_t n); diff --git a/test/pacman/tests/fileconflict007.py b/test/pacman/tests/fileconflict007.py index b61ddb4..7fe65ed 100644 --- a/test/pacman/tests/fileconflict007.py +++ b/test/pacman/tests/fileconflict007.py @@ -16,5 +16,3 @@ self.addrule("PKG_EXIST=pkg") self.addrule("PKG_VERSION=pkg|1.0-2") self.addrule("FILE_TYPE=dir/symdir/|dir") - -self.expectfailure = True diff --git a/test/pacman/tests/sync701.py b/test/pacman/tests/sync701.py index 912c794..590845f 100644 --- a/test/pacman/tests/sync701.py +++ b/test/pacman/tests/sync701.py @@ -19,5 +19,3 @@ self.addrule("PKG_VERSION=pkg1|1.0-2") self.addrule("PKG_EXIST=pkg2") self.addrule("FILE_TYPE=lib|dir") - -self.expectfailure = True diff --git a/test/pacman/tests/sync702.py b/test/pacman/tests/sync702.py index 8f4c0ad..c3e2320 100644 --- a/test/pacman/tests/sync702.py +++ b/test/pacman/tests/sync702.py @@ -19,5 +19,3 @@ self.addrule("PKG_VERSION=pkg2|1.0-2") self.addrule("PKG_EXIST=pkg1") self.addrule("FILE_TYPE=lib|dir") - -self.expectfailure = True -- 1.8.2.1
On 27/04/13 10:00, Andrew Gregory wrote:
In the event of a symlink<->dir transition, the paths for the incoming package need to be resolved to match the filelist for installed packages.
Can you explain this to me? Every time I think I understand, I don't... e.g. outdated local package: /usr/foo -> /usr/bar/ /usr/bar/baz update sync package: /usr/foo/baz So /usr/foo/baz is a new file so it is tested for presence in the filesystem. But /usr/foo/baz "exists" on the filesystem as /usr/bar/baz.
From what I understand, you resolve the package file "/usr/foo/baz" to "/usr/bar/baz" and note that it is in the old package so not a conflict. Which feels wrong to me...
Can we do something like testing whether the directory of the "conflicting" file and the resolved path of that directory are different? If not, then we must be dealing with a case of a filesystem symlink being replaced with a package directory (otherwise the package directory would have conflicted with the filesystem symlink earlier).
Note: this only enables the symlink<->dir transition itself. Any installed packages with invalid file lists containing symlinks are not detected.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/conflict.c | 42 ++++++++++++++++------------------- src/common/util-common.c | 43 ++++++++++++++++++++++++++++++++++++ src/common/util-common.h | 1 + test/pacman/tests/fileconflict007.py | 2 -- test/pacman/tests/sync701.py | 2 -- test/pacman/tests/sync702.py | 2 -- 6 files changed, 63 insertions(+), 29 deletions(-)
diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 518a2c6..08c1cbe 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -501,7 +501,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, /* have we acted on this conflict? */ int resolved_conflict = 0; struct stat lsbuf; - char path[PATH_MAX]; + char path[PATH_MAX], resolved_path[PATH_MAX]; size_t pathlen;
pathlen = snprintf(path, PATH_MAX, "%s%s", handle->root, filestr); @@ -513,7 +513,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle,
_alpm_log(handle, ALPM_LOG_DEBUG, "checking possible conflict: %s\n", path);
- if(filestr[strlen(filestr) - 1] == '/') { + if(path[pathlen - 1] == '/') { if(S_ISDIR(lsbuf.st_mode)) { _alpm_log(handle, ALPM_LOG_DEBUG, "file is a directory, not a conflict\n"); continue; @@ -524,7 +524,19 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, path[pathlen - 1] = '\0'; }
- relative_path = path + rootlen; + if(lrealpath(path, resolved_path) + && strncmp(resolved_path, handle->root, rootlen) == 0) { + relative_path = resolved_path + rootlen; + } else { + relative_path = path + rootlen; + } + + if(!resolved_conflict && dbpkg + && alpm_filelist_contains(alpm_pkg_get_files(dbpkg), relative_path)) { + _alpm_log(handle, ALPM_LOG_DEBUG, + "package contained the resolved realpath\n"); + resolved_conflict = 1; + }
/* Check remove list (will we remove the conflicting local file?) */ for(k = rem; k && !resolved_conflict; k = k->next) { @@ -546,12 +558,12 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, alpm_pkg_t *localp2 = _alpm_db_get_pkgfromcache(handle->db_local, p2->name);
/* localp2->files will be removed (target conflicts are handled by CHECK 1) */ - if(localp2 && alpm_filelist_contains(alpm_pkg_get_files(localp2), filestr)) { + if(localp2 && alpm_filelist_contains(alpm_pkg_get_files(localp2), relative_path)) { /* skip removal of file, but not add. this will prevent a second * package from removing the file when it was already installed * by its new owner (whether the file is in backup array or not */ handle->trans->skip_remove = - alpm_list_add(handle->trans->skip_remove, strdup(filestr)); + alpm_list_add(handle->trans->skip_remove, strdup(relative_path)); _alpm_log(handle, ALPM_LOG_DEBUG, "file changed packages, adding to remove skiplist\n"); resolved_conflict = 1; @@ -560,8 +572,8 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle,
/* check if all files of the dir belong to the installed pkg */ if(!resolved_conflict && S_ISDIR(lsbuf.st_mode) && dbpkg) { - char *dir = malloc(strlen(filestr) + 2); - sprintf(dir, "%s/", filestr); + char *dir = malloc(strlen(relative_path) + 2); + sprintf(dir, "%s/", relative_path); if(alpm_filelist_contains(alpm_pkg_get_files(dbpkg), dir)) { _alpm_log(handle, ALPM_LOG_DEBUG, "checking if all files in %s belong to %s\n", @@ -571,22 +583,6 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, free(dir); }
- /* check if a component of the filepath was a link. canonicalize the path - * and look for it in the old package. note that the actual file under - * consideration cannot itself be a link, as it might be unowned- path - * components can be safely checked as all directories are "unowned". */ - if(!resolved_conflict && dbpkg && !S_ISLNK(lsbuf.st_mode)) { - char rpath[PATH_MAX]; - if(realpath(path, rpath)) { - const char *relative_rpath = rpath + rootlen; - if(alpm_filelist_contains(alpm_pkg_get_files(dbpkg), relative_rpath)) { - _alpm_log(handle, ALPM_LOG_DEBUG, - "package contained the resolved realpath\n"); - resolved_conflict = 1; - } - } - } - /* is the file unowned and in the backup list of the new package? */ if(!resolved_conflict && _alpm_needbackup(filestr, p1)) { alpm_list_t *local_pkgs = _alpm_db_get_pkgcache(handle->db_local); diff --git a/src/common/util-common.c b/src/common/util-common.c index 7145e49..c2422e0 100644 --- a/src/common/util-common.c +++ b/src/common/util-common.c @@ -73,6 +73,49 @@ char *mdirname(const char *path) return strdup("."); }
+/** Resolve the canonicalized absolute path of a symlink. + * @param path path to resolve + * @param resolved_path destination for the resolved path, will be malloc'd if + * NULL + * @return the resolved path + */ +char *lrealpath(const char *path, char *resolved_path) +{ + const char *bname = mbasename(path); + char *rpath = NULL, *dname = NULL; + int success = 0; + + if(strcmp(path, ".") == 0 || strcmp(path, "..") == 0) { + /* the entire path needs to be resolved */ + return realpath(path, resolved_path); + } + + if(!(dname = mdirname(path))) { + goto cleanup; + } + if(!(rpath = realpath(dname, NULL))) { + goto cleanup; + } + if(!resolved_path) { + if(!(resolved_path = malloc(strlen(rpath) + strlen(bname) + 2))) { + goto cleanup; + } + } + + strcpy(resolved_path, rpath); + if(resolved_path[strlen(resolved_path) - 1] != '/') { + strcat(resolved_path, "/"); + } + strcat(resolved_path, bname); + success = 1; + +cleanup: + free(dname); + free(rpath); + + return (success ? resolved_path : NULL); +} + #ifndef HAVE_STRNDUP /* A quick and dirty implementation derived from glibc */ /** Determines the length of a fixed-size string. diff --git a/src/common/util-common.h b/src/common/util-common.h index 04d4e9d..b3a4df1 100644 --- a/src/common/util-common.h +++ b/src/common/util-common.h @@ -22,6 +22,7 @@
const char *mbasename(const char *path); char *mdirname(const char *path); +char *lrealpath(const char *path, char *resolved_path);
#ifndef HAVE_STRNDUP char *strndup(const char *s, size_t n); diff --git a/test/pacman/tests/fileconflict007.py b/test/pacman/tests/fileconflict007.py index b61ddb4..7fe65ed 100644 --- a/test/pacman/tests/fileconflict007.py +++ b/test/pacman/tests/fileconflict007.py @@ -16,5 +16,3 @@ self.addrule("PKG_EXIST=pkg") self.addrule("PKG_VERSION=pkg|1.0-2") self.addrule("FILE_TYPE=dir/symdir/|dir") - -self.expectfailure = True diff --git a/test/pacman/tests/sync701.py b/test/pacman/tests/sync701.py index 912c794..590845f 100644 --- a/test/pacman/tests/sync701.py +++ b/test/pacman/tests/sync701.py @@ -19,5 +19,3 @@ self.addrule("PKG_VERSION=pkg1|1.0-2") self.addrule("PKG_EXIST=pkg2") self.addrule("FILE_TYPE=lib|dir") - -self.expectfailure = True diff --git a/test/pacman/tests/sync702.py b/test/pacman/tests/sync702.py index 8f4c0ad..c3e2320 100644 --- a/test/pacman/tests/sync702.py +++ b/test/pacman/tests/sync702.py @@ -19,5 +19,3 @@ self.addrule("PKG_VERSION=pkg2|1.0-2") self.addrule("PKG_EXIST=pkg1") self.addrule("FILE_TYPE=lib|dir") - -self.expectfailure = True
On 04/29/13 at 12:33am, Allan McRae wrote:
On 27/04/13 10:00, Andrew Gregory wrote:
In the event of a symlink<->dir transition, the paths for the incoming package need to be resolved to match the filelist for installed packages.
Can you explain this to me? Every time I think I understand, I don't...
e.g.
outdated local package: /usr/foo -> /usr/bar/ /usr/bar/baz
update sync package: /usr/foo/baz
So /usr/foo/baz is a new file so it is tested for presence in the filesystem. But /usr/foo/baz "exists" on the filesystem as /usr/bar/baz.
From what I understand, you resolve the package file "/usr/foo/baz" to "/usr/bar/baz" and note that it is in the old package so not a conflict. Which feels wrong to me...
Why would that be wrong? /usr/foo itself isn't a conflict and both /usr/foo and /usr/bar/baz will both be removed/overwritten in the upgrade process so /usr/foo/baz should be fine, should it not? In fact, we already do what you outlined (see the code below), this patch just does the resolution upfront so the real path can be used for the rest of the checks as well (ie "changing hands" from one package to another).
- /* check if a component of the filepath was a link. canonicalize the path - * and look for it in the old package. note that the actual file under - * consideration cannot itself be a link, as it might be unowned- path - * components can be safely checked as all directories are "unowned". */ - if(!resolved_conflict && dbpkg && !S_ISLNK(lsbuf.st_mode)) { - char rpath[PATH_MAX]; - if(realpath(path, rpath)) { - const char *relative_rpath = rpath + rootlen; - if(alpm_filelist_contains(alpm_pkg_get_files(dbpkg), relative_rpath)) { - _alpm_log(handle, ALPM_LOG_DEBUG, - "package contained the resolved realpath\n"); - resolved_conflict = 1; - } - } - } -
On 29/04/13 00:59, Andrew Gregory wrote:
On 04/29/13 at 12:33am, Allan McRae wrote:
On 27/04/13 10:00, Andrew Gregory wrote:
In the event of a symlink<->dir transition, the paths for the incoming package need to be resolved to match the filelist for installed packages.
Can you explain this to me? Every time I think I understand, I don't...
e.g.
outdated local package: /usr/foo -> /usr/bar/ /usr/bar/baz
update sync package: /usr/foo/baz
So /usr/foo/baz is a new file so it is tested for presence in the filesystem. But /usr/foo/baz "exists" on the filesystem as /usr/bar/baz.
From what I understand, you resolve the package file "/usr/foo/baz" to "/usr/bar/baz" and note that it is in the old package so not a conflict. Which feels wrong to me...
Why would that be wrong? /usr/foo itself isn't a conflict and both /usr/foo and /usr/bar/baz will both be removed/overwritten in the upgrade process so /usr/foo/baz should be fine, should it not?
In fact, we already do what you outlined (see the code below), this patch just does the resolution upfront so the real path can be used for the rest of the checks as well (ie "changing hands" from one package to another).
My point is, why do we need to do that at all? Using my example again: outdated local package: /usr/foo -> /usr/bar/ /usr/bar/baz update sync package: /usr/foo/baz Initially /usr/foo/baz is flagged as a potential conflict. But because the realpath of /usr/foo/ is not /usr/foo, we know there must be a symlink to a directory somewhere in that path. So, we know the symlink is being replaced with a directory and either: 1) it can not be and has already been flagged as a conflict or 2) all files in this new directory are new and can not be conflicts. Allan
After the initial checks, we either use the path as a directory and have to append the trailing slash anyway or use it as a file in which case the trailing slash should be excluded. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/conflict.c | 12 ++++++------ test/pacman/tests/sync701.py | 2 -- test/pacman/tests/sync702.py | 2 -- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 3121cc7..d44a459 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -548,12 +548,12 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, localp2 = _alpm_db_get_pkgfromcache(handle->db_local, p2->name); /* localp2->files will be removed (target conflicts are handled by CHECK 1) */ - if(localp2 && alpm_filelist_contains(alpm_pkg_get_files(localp2), filestr)) { + if(localp2 && alpm_filelist_contains(alpm_pkg_get_files(localp2), relative_path)) { /* skip removal of file, but not add. this will prevent a second * package from removing the file when it was already installed * by its new owner (whether the file is in backup array or not */ handle->trans->skip_remove = - alpm_list_add(handle->trans->skip_remove, strdup(filestr)); + alpm_list_add(handle->trans->skip_remove, strdup(relative_path)); _alpm_log(handle, ALPM_LOG_DEBUG, "file changed packages, adding to remove skiplist\n"); resolved_conflict = 1; @@ -562,8 +562,8 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, /* check if all files of the dir belong to the installed pkg */ if(!resolved_conflict && S_ISDIR(lsbuf.st_mode) && dbpkg) { - char *dir = malloc(strlen(filestr) + 2); - sprintf(dir, "%s/", filestr); + char *dir = malloc(strlen(relative_path) + 2); + sprintf(dir, "%s/", relative_path); if(alpm_filelist_contains(alpm_pkg_get_files(dbpkg), dir)) { _alpm_log(handle, ALPM_LOG_DEBUG, "checking if all files in %s belong to %s\n", @@ -590,11 +590,11 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, } /* is the file unowned and in the backup list of the new package? */ - if(!resolved_conflict && _alpm_needbackup(filestr, p1)) { + if(!resolved_conflict && _alpm_needbackup(relative_path, p1)) { alpm_list_t *local_pkgs = _alpm_db_get_pkgcache(handle->db_local); int found = 0; for(k = local_pkgs; k && !found; k = k->next) { - if(alpm_filelist_contains(alpm_pkg_get_files(k->data), filestr)) { + if(alpm_filelist_contains(alpm_pkg_get_files(k->data), relative_path)) { found = 1; } } diff --git a/test/pacman/tests/sync701.py b/test/pacman/tests/sync701.py index 912c794..590845f 100644 --- a/test/pacman/tests/sync701.py +++ b/test/pacman/tests/sync701.py @@ -19,5 +19,3 @@ self.addrule("PKG_VERSION=pkg1|1.0-2") self.addrule("PKG_EXIST=pkg2") self.addrule("FILE_TYPE=lib|dir") - -self.expectfailure = True diff --git a/test/pacman/tests/sync702.py b/test/pacman/tests/sync702.py index 8f4c0ad..c3e2320 100644 --- a/test/pacman/tests/sync702.py +++ b/test/pacman/tests/sync702.py @@ -19,5 +19,3 @@ self.addrule("PKG_VERSION=pkg2|1.0-2") self.addrule("PKG_EXIST=pkg1") self.addrule("FILE_TYPE=lib|dir") - -self.expectfailure = True -- 1.8.2.2
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lrealpath definition moved to query_fileowner patch. lib/libalpm/conflict.c | 39 ++++++++++++++++++------------------ test/pacman/tests/fileconflict007.py | 2 -- test/pacman/tests/symlink020.py | 20 ++++++++++++++++++ 3 files changed, 40 insertions(+), 21 deletions(-) create mode 100644 test/pacman/tests/symlink020.py diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index d44a459..ab9546c 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -505,6 +505,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, size_t pathlen; pathlen = snprintf(path, PATH_MAX, "%s%s", handle->root, filestr); + relative_path = path + rootlen; /* stat the file - if it exists, do some checks */ if(_alpm_lstat(path, &lsbuf) != 0) { @@ -513,7 +514,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, _alpm_log(handle, ALPM_LOG_DEBUG, "checking possible conflict: %s\n", path); - if(filestr[strlen(filestr) - 1] == '/') { + if(path[pathlen - 1] == '/') { if(S_ISDIR(lsbuf.st_mode)) { _alpm_log(handle, ALPM_LOG_DEBUG, "file is a directory, not a conflict\n"); continue; @@ -522,9 +523,25 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, * not include the trailing slash. This allows things like file -> * directory replacements. */ path[pathlen - 1] = '\0'; - } - relative_path = path + rootlen; + /* Check if the directory was a file in dbpkg */ + if(alpm_filelist_contains(alpm_pkg_get_files(dbpkg), relative_path)) { + size_t fslen = strlen(filestr); + _alpm_log(handle, ALPM_LOG_DEBUG, + "replacing package file with a directory, not a conflict\n"); + resolved_conflict = 1; + + /* go ahead and skip any files inside filestr as they will + * necessarily be resolved by replacing the file with a dir + * NOTE: afterward, j will point to the last file inside filestr */ + for( ; j->next; j = j->next) { + const char *filestr2 = j->next->data; + if(strncmp(filestr, filestr2, fslen) != 0) { + break; + } + } + } + } /* Check remove list (will we remove the conflicting local file?) */ for(k = rem; k && !resolved_conflict; k = k->next) { @@ -573,22 +590,6 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, free(dir); } - /* check if a component of the filepath was a link. canonicalize the path - * and look for it in the old package. note that the actual file under - * consideration cannot itself be a link, as it might be unowned- path - * components can be safely checked as all directories are "unowned". */ - if(!resolved_conflict && dbpkg && !S_ISLNK(lsbuf.st_mode)) { - char rpath[PATH_MAX]; - if(realpath(path, rpath)) { - const char *relative_rpath = rpath + rootlen; - if(alpm_filelist_contains(alpm_pkg_get_files(dbpkg), relative_rpath)) { - _alpm_log(handle, ALPM_LOG_DEBUG, - "package contained the resolved realpath\n"); - resolved_conflict = 1; - } - } - } - /* is the file unowned and in the backup list of the new package? */ if(!resolved_conflict && _alpm_needbackup(relative_path, p1)) { alpm_list_t *local_pkgs = _alpm_db_get_pkgcache(handle->db_local); diff --git a/test/pacman/tests/fileconflict007.py b/test/pacman/tests/fileconflict007.py index b61ddb4..7fe65ed 100644 --- a/test/pacman/tests/fileconflict007.py +++ b/test/pacman/tests/fileconflict007.py @@ -16,5 +16,3 @@ self.addrule("PKG_EXIST=pkg") self.addrule("PKG_VERSION=pkg|1.0-2") self.addrule("FILE_TYPE=dir/symdir/|dir") - -self.expectfailure = True diff --git a/test/pacman/tests/symlink020.py b/test/pacman/tests/symlink020.py new file mode 100644 index 0000000..343add2 --- /dev/null +++ b/test/pacman/tests/symlink020.py @@ -0,0 +1,20 @@ +self.description = "symlink -> dir replacment" + +lp1 = pmpkg("pkg1") +lp1.files = ["usr/lib/foo", + "lib -> usr/lib/"] +self.addpkg2db("local", lp1) + +lp1 = pmpkg("pkg2") +lp1.files = ["usr/lib/bar"] +self.addpkg2db("local", lp1) + +sp = pmpkg("pkg1", "1.0-2") +sp.files = ["lib/bar"] +self.addpkg2db("sync", sp) + +self.args = "-S %s" % sp.name + +self.addrule("PACMAN_RETCODE=0") +self.addrule("FILE_TYPE=lib|dir") +self.addrule("FILE_TYPE=lib/bar|file") -- 1.8.2.2
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/add.c | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 1d9db60..0b8aedf 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -204,19 +204,17 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, * D | 10 | 11 | 12 * * 1,2,3- extract, no magic necessary. lstat (_alpm_lstat) will fail here. - * 4,5,6,7,8- conflict checks should have caught this. either overwrite + * 4,5,6,7,8,9- conflict checks should have caught this. either overwrite * or backup the file. - * 9- follow the symlink, hopefully it is a directory, check it. - * 10- file replacing directory- don't allow it. - * 11- don't extract symlink- a dir exists here. we don't want links to - * links, etc. + * 10,11- file replacing directory- don't allow it. * 12- skip extraction, dir already exists. */ - /* do both a lstat and a stat, so we can see what symlinks point to */ struct stat lsbuf, sbuf; - if(_alpm_lstat(filename, &lsbuf) != 0 || stat(filename, &sbuf) != 0) { - /* cases 1,2,3: couldn't stat an existing file, skip all backup checks */ + if(stat(filename, &sbuf) != 0 || _alpm_lstat(filename, &lsbuf) != 0) { + /* cases 1,2,3: file doesn't exist or is a broken symlink, + * skip all backup checks */ + /* TODO: should we really treat broken symlinks as if they don't exist? */ } else { if(S_ISDIR(lsbuf.st_mode)) { if(S_ISDIR(entrymode)) { @@ -243,23 +241,8 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, archive_read_data_skip(archive); return 1; } - } else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(entrymode)) { - /* case 9: existing symlink, dir in package */ - if(S_ISDIR(sbuf.st_mode)) { - /* the symlink on FS is to a directory, so we'll use it */ - _alpm_log(handle, ALPM_LOG_DEBUG, "extract: skipping symlink overwrite of %s\n", - filename); - archive_read_data_skip(archive); - return 0; - } else { - /* this is BAD. symlink was not to a directory */ - _alpm_log(handle, ALPM_LOG_ERROR, _("extract: symlink %s does not point to dir\n"), - filename); - archive_read_data_skip(archive); - return 1; - } - } else if(S_ISREG(lsbuf.st_mode) && S_ISDIR(entrymode)) { - /* case 6: trying to overwrite file with dir */ + } else if(!S_ISDIR(lsbuf.st_mode) && S_ISDIR(entrymode)) { + /* case 6,9: trying to overwrite file with dir */ _alpm_log(handle, ALPM_LOG_DEBUG, "extract: overwriting file with dir %s\n", filename); } else if(S_ISREG(entrymode)) { -- 1.8.2.1
On 27/04/13 10:00, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/add.c | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-)
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 1d9db60..0b8aedf 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -204,19 +204,17 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, * D | 10 | 11 | 12 * * 1,2,3- extract, no magic necessary. lstat (_alpm_lstat) will fail here. - * 4,5,6,7,8- conflict checks should have caught this. either overwrite + * 4,5,6,7,8,9- conflict checks should have caught this. either overwrite * or backup the file. - * 9- follow the symlink, hopefully it is a directory, check it. - * 10- file replacing directory- don't allow it. - * 11- don't extract symlink- a dir exists here. we don't want links to - * links, etc. + * 10,11- file replacing directory- don't allow it. * 12- skip extraction, dir already exists. */
- /* do both a lstat and a stat, so we can see what symlinks point to */ struct stat lsbuf, sbuf; - if(_alpm_lstat(filename, &lsbuf) != 0 || stat(filename, &sbuf) != 0) { - /* cases 1,2,3: couldn't stat an existing file, skip all backup checks */ + if(stat(filename, &sbuf) != 0 || _alpm_lstat(filename, &lsbuf) != 0) { + /* cases 1,2,3: file doesn't exist or is a broken symlink, + * skip all backup checks */ + /* TODO: should we really treat broken symlinks as if they don't exist? */
NO! We definitely should not... In fact, I'd like to see this patch result in this: * (F=file, N=node, S=symlink, D=dir) * | F/N/S | D * non-existent | 1 | 2 * F/N/S | 3 | 4 * D | 5 | 6 because now there is no difference between a file and a symlink. Which probably means getting rid of the lstat call (although I did not look at the code to confirm...)
} else { if(S_ISDIR(lsbuf.st_mode)) { if(S_ISDIR(entrymode)) { @@ -243,23 +241,8 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, archive_read_data_skip(archive); return 1; } - } else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(entrymode)) { - /* case 9: existing symlink, dir in package */ - if(S_ISDIR(sbuf.st_mode)) { - /* the symlink on FS is to a directory, so we'll use it */ - _alpm_log(handle, ALPM_LOG_DEBUG, "extract: skipping symlink overwrite of %s\n", - filename); - archive_read_data_skip(archive); - return 0; - } else { - /* this is BAD. symlink was not to a directory */ - _alpm_log(handle, ALPM_LOG_ERROR, _("extract: symlink %s does not point to dir\n"), - filename); - archive_read_data_skip(archive); - return 1; - } - } else if(S_ISREG(lsbuf.st_mode) && S_ISDIR(entrymode)) { - /* case 6: trying to overwrite file with dir */ + } else if(!S_ISDIR(lsbuf.st_mode) && S_ISDIR(entrymode)) { + /* case 6,9: trying to overwrite file with dir */ _alpm_log(handle, ALPM_LOG_DEBUG, "extract: overwriting file with dir %s\n", filename); } else if(S_ISREG(entrymode)) {
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- Note the newly modified test at the bottom. lib/libalpm/add.c | 56 +++++++++++++---------------------------- test/pacman/tests/upgrade045.py | 2 +- 2 files changed, 18 insertions(+), 40 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 3ef81e3..49f36ce 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -197,30 +197,25 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, * to get 'right'. Here are the possibilities, with the filesystem * on the left and the package on the top: * (F=file, N=node, S=symlink, D=dir) - * | F/N | S | D - * non-existent | 1 | 2 | 3 - * F/N | 4 | 5 | 6 - * S | 7 | 8 | 9 - * D | 10 | 11 | 12 + * | F/N | D + * non-existent | 1 | 2 + * F/N | 3 | 4 + * D | 5 | 6 * - * 1,2,3- extract, no magic necessary. lstat (_alpm_lstat) will fail here. - * 4,5,6,7,8- conflict checks should have caught this. either overwrite + * 1,2- extract, no magic necessary. lstat (_alpm_lstat) will fail here. + * 3,4- conflict checks should have caught this. either overwrite * or backup the file. - * 9- follow the symlink, hopefully it is a directory, check it. - * 10- file replacing directory- don't allow it. - * 11- don't extract symlink- a dir exists here. we don't want links to - * links, etc. - * 12- skip extraction, dir already exists. + * 5- file replacing directory- don't allow it. + * 6- skip extraction, dir already exists. */ - /* do both a lstat and a stat, so we can see what symlinks point to */ - struct stat lsbuf, sbuf; - if(_alpm_lstat(filename, &lsbuf) != 0 || stat(filename, &sbuf) != 0) { - /* cases 1,2,3: couldn't stat an existing file, skip all backup checks */ + struct stat lsbuf; + if(_alpm_lstat(filename, &lsbuf) != 0) { + /* cases 1,2: file doesn't exist, skip all backup checks */ } else { if(S_ISDIR(lsbuf.st_mode)) { if(S_ISDIR(entrymode)) { - /* case 12: existing dir, ignore it */ + /* case 6: existing dir, ignore it */ if(lsbuf.st_mode != entrymode) { /* if filesystem perms are different than pkg perms, warn user */ mode_t mask = 07777; @@ -237,33 +232,18 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, archive_read_data_skip(archive); return 0; } else { - /* case 10/11: trying to overwrite dir with file/symlink, don't allow it */ + /* case 5: trying to overwrite dir with file, don't allow it */ _alpm_log(handle, ALPM_LOG_ERROR, _("extract: not overwriting dir with file %s\n"), filename); archive_read_data_skip(archive); return 1; } - } else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(entrymode)) { - /* case 9: existing symlink, dir in package */ - if(S_ISDIR(sbuf.st_mode)) { - /* the symlink on FS is to a directory, so we'll use it */ - _alpm_log(handle, ALPM_LOG_DEBUG, "extract: skipping symlink overwrite of %s\n", - filename); - archive_read_data_skip(archive); - return 0; - } else { - /* this is BAD. symlink was not to a directory */ - _alpm_log(handle, ALPM_LOG_ERROR, _("extract: symlink %s does not point to dir\n"), - filename); - archive_read_data_skip(archive); - return 1; - } - } else if(S_ISREG(lsbuf.st_mode) && S_ISDIR(entrymode)) { - /* case 6: trying to overwrite file with dir */ + } else if(S_ISDIR(entrymode)) { + /* case 4: trying to overwrite file with dir */ _alpm_log(handle, ALPM_LOG_DEBUG, "extract: overwriting file with dir %s\n", filename); - } else if(S_ISREG(entrymode)) { - /* case 4,7: */ + } else { + /* case 3: */ /* if file is in NoUpgrade, don't touch it */ if(alpm_list_find(handle->noupgrade, entryname, _alpm_fnmatch)) { notouch = 1; @@ -288,8 +268,6 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, } } } - /* else if(S_ISLNK(entrymode)) */ - /* case 5,8: don't need to do anything special */ } /* we need access to the original entryname later after calls to diff --git a/test/pacman/tests/upgrade045.py b/test/pacman/tests/upgrade045.py index 3b7f476..e165ba3 100644 --- a/test/pacman/tests/upgrade045.py +++ b/test/pacman/tests/upgrade045.py @@ -14,4 +14,4 @@ self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_VERSION=foo|1.0-2") -self.addrule("FILE_EXIST=etc/foo.cfg") +self.addrule("LINK_EXIST=etc/foo.cfg") -- 1.8.2.2
On 11/05/13 12:21, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
Note the newly modified test at the bottom.
lib/libalpm/add.c | 56 +++++++++++++---------------------------- test/pacman/tests/upgrade045.py | 2 +- 2 files changed, 18 insertions(+), 40 deletions(-)
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 3ef81e3..49f36ce 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -197,30 +197,25 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, * to get 'right'. Here are the possibilities, with the filesystem * on the left and the package on the top: * (F=file, N=node, S=symlink, D=dir) - * | F/N | S | D - * non-existent | 1 | 2 | 3 - * F/N | 4 | 5 | 6 - * S | 7 | 8 | 9 - * D | 10 | 11 | 12 + * | F/N | D + * non-existent | 1 | 2 + * F/N | 3 | 4 + * D | 5 | 6 * - * 1,2,3- extract, no magic necessary. lstat (_alpm_lstat) will fail here. - * 4,5,6,7,8- conflict checks should have caught this. either overwrite + * 1,2- extract, no magic necessary. lstat (_alpm_lstat) will fail here. + * 3,4- conflict checks should have caught this. either overwrite * or backup the file. - * 9- follow the symlink, hopefully it is a directory, check it. - * 10- file replacing directory- don't allow it. - * 11- don't extract symlink- a dir exists here. we don't want links to - * links, etc. - * 12- skip extraction, dir already exists. + * 5- file replacing directory- don't allow it. + * 6- skip extraction, dir already exists. */
- /* do both a lstat and a stat, so we can see what symlinks point to */ - struct stat lsbuf, sbuf; - if(_alpm_lstat(filename, &lsbuf) != 0 || stat(filename, &sbuf) != 0) { - /* cases 1,2,3: couldn't stat an existing file, skip all backup checks */ + struct stat lsbuf; + if(_alpm_lstat(filename, &lsbuf) != 0) { + /* cases 1,2: file doesn't exist, skip all backup checks */ } else { if(S_ISDIR(lsbuf.st_mode)) { if(S_ISDIR(entrymode)) { - /* case 12: existing dir, ignore it */ + /* case 6: existing dir, ignore it */ if(lsbuf.st_mode != entrymode) { /* if filesystem perms are different than pkg perms, warn user */ mode_t mask = 07777; @@ -237,33 +232,18 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, archive_read_data_skip(archive); return 0; } else { - /* case 10/11: trying to overwrite dir with file/symlink, don't allow it */ + /* case 5: trying to overwrite dir with file, don't allow it */ _alpm_log(handle, ALPM_LOG_ERROR, _("extract: not overwriting dir with file %s\n"), filename); archive_read_data_skip(archive); return 1; } - } else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(entrymode)) { - /* case 9: existing symlink, dir in package */ - if(S_ISDIR(sbuf.st_mode)) { - /* the symlink on FS is to a directory, so we'll use it */ - _alpm_log(handle, ALPM_LOG_DEBUG, "extract: skipping symlink overwrite of %s\n", - filename); - archive_read_data_skip(archive); - return 0; - } else { - /* this is BAD. symlink was not to a directory */ - _alpm_log(handle, ALPM_LOG_ERROR, _("extract: symlink %s does not point to dir\n"), - filename); - archive_read_data_skip(archive); - return 1; - } - } else if(S_ISREG(lsbuf.st_mode) && S_ISDIR(entrymode)) { - /* case 6: trying to overwrite file with dir */ + } else if(S_ISDIR(entrymode)) { + /* case 4: trying to overwrite file with dir */ _alpm_log(handle, ALPM_LOG_DEBUG, "extract: overwriting file with dir %s\n", filename); - } else if(S_ISREG(entrymode)) { - /* case 4,7: */ + } else { + /* case 3: */ /* if file is in NoUpgrade, don't touch it */ if(alpm_list_find(handle->noupgrade, entryname, _alpm_fnmatch)) { notouch = 1; @@ -288,8 +268,6 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, } } } - /* else if(S_ISLNK(entrymode)) */ - /* case 5,8: don't need to do anything special */ }
/* we need access to the original entryname later after calls to diff --git a/test/pacman/tests/upgrade045.py b/test/pacman/tests/upgrade045.py index 3b7f476..e165ba3 100644 --- a/test/pacman/tests/upgrade045.py +++ b/test/pacman/tests/upgrade045.py @@ -14,4 +14,4 @@
self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_VERSION=foo|1.0-2") -self.addrule("FILE_EXIST=etc/foo.cfg") +self.addrule("LINK_EXIST=etc/foo.cfg")
Looks good. For that pactest, add a FILE_EXISTS=/etc/foo.cfg.pacnew too. Also, in that pactest: p1.files = ["etc/foo.cfg*"] what is the "*" for there?
We always want to work with the package file itself, not its target if it's a symlink. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/remove.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index 652aa50..0b4f80c 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -442,11 +442,7 @@ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg, return 1; } - /* we want to do a lstat here, and not a _alpm_lstat. - * if a directory in the package is actually a directory symlink on the - * filesystem, we want to work with the linked directory instead of the - * actual symlink */ - if(lstat(file, &buf)) { + if(_alpm_lstat(file, &buf)) { _alpm_log(handle, ALPM_LOG_DEBUG, "file %s does not exist\n", file); return 1; } -- 1.8.2.1
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/query.c | 90 +++++++++++------------------------------------------- 1 file changed, 17 insertions(+), 73 deletions(-) diff --git a/src/pacman/query.c b/src/pacman/query.c index f051571..6befcae 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -133,11 +133,11 @@ static int query_fileowner(alpm_list_t *targets) packages = alpm_db_get_pkgcache(db_local); for(t = targets; t; t = alpm_list_next(t)) { - char *filename = NULL, *dname = NULL, *rpath = NULL; - const char *bname; + char *filename = NULL; + char rpath[PATH_MAX], *rel_path; struct stat buf; alpm_list_t *i; - size_t len, is_dir, bname_len, pbname_len; + size_t len, is_dir; unsigned int found = 0; if((filename = strdup(t->data)) == NULL) { @@ -165,82 +165,28 @@ static int query_fileowner(alpm_list_t *targets) } } - is_dir = S_ISDIR(buf.st_mode) ? 1 : 0; - if(is_dir) { - /* the entire filename is safe to resolve if we know it points to a dir, - * and it's necessary in case it ends in . or .. */ - dname = realpath(filename, NULL); - bname = mbasename(dname); - rpath = mdirname(dname); - } else { - bname = mbasename(filename); - dname = mdirname(filename); - rpath = realpath(dname, NULL); - } - - bname_len = strlen(bname); - pbname_len = bname_len + is_dir; - - if(!dname || !rpath) { + if(!lrealpath(filename, rpath)) { pm_printf(ALPM_LOG_ERROR, _("cannot determine real path for '%s': %s\n"), filename, strerror(errno)); goto targcleanup; } - for(i = packages; i && (!found || is_dir); i = alpm_list_next(i)) { - alpm_pkg_t *info = i->data; - alpm_filelist_t *filelist = alpm_pkg_get_files(info); - size_t j; - - for(j = 0; j < filelist->count; j++) { - const alpm_file_t *file = filelist->files + j; - char *ppath; - const char *pkgfile = file->name; - size_t pkgfile_len = strlen(pkgfile); - - /* make sure pkgfile and target are of the same type */ - if(is_dir != (pkgfile[pkgfile_len - 1] == '/')) { - continue; - } - - /* make sure pkgfile is long enough */ - if(pkgfile_len < pbname_len) { - continue; - } - - /* make sure pbname_len takes us to the start of a path component */ - if(pbname_len != pkgfile_len && pkgfile[pkgfile_len - pbname_len - 1] != '/') { - continue; - } + if(strncmp(rpath, path, rootlen) != 0) { + /* file is outside root, we know nothing can own it */ + pm_printf(ALPM_LOG_ERROR, _("No package owns %s\n"), filename); + goto targcleanup; + } - /* compare basename with bname */ - if(strncmp(pkgfile + pkgfile_len - pbname_len, bname, bname_len) != 0) { - continue; - } + rel_path = rpath + rootlen; - /* concatenate our dirname and the root path */ - if(rootlen + 1 + pkgfile_len - pbname_len > PATH_MAX) { - path[rootlen] = '\0'; /* reset path for error message */ - pm_printf(ALPM_LOG_ERROR, _("path too long: %s%s\n"), path, pkgfile); - continue; - } - strncpy(path + rootlen, pkgfile, pkgfile_len - pbname_len); - path[rootlen + pkgfile_len - pbname_len] = '\0'; - - ppath = realpath(path, NULL); - if(!ppath) { - pm_printf(ALPM_LOG_ERROR, _("cannot determine real path for '%s': %s\n"), - path, strerror(errno)); - continue; - } + if((is_dir = S_ISDIR(buf.st_mode))) { + strcat(rpath, "/"); + } - if(strcmp(ppath, rpath) == 0) { - print_query_fileowner(filename, info); - found = 1; - free(ppath); - break; - } - free(ppath); + for(i = packages; i && (!found || is_dir); i = alpm_list_next(i)) { + if(alpm_filelist_contains(alpm_pkg_get_files(i->data), rel_path)) { + print_query_fileowner(rpath, i->data); + found = 1; } } if(!found) { @@ -252,8 +198,6 @@ targcleanup: ret++; } free(filename); - free(rpath); - free(dname); } return ret; -- 1.8.2.1
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- Updated to include lrealpath definition and a buffer overflow check.. src/pacman/query.c | 137 +++++++++++++++++++++++++---------------------------- 1 file changed, 64 insertions(+), 73 deletions(-) diff --git a/src/pacman/query.c b/src/pacman/query.c index f051571..f5862a2 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -90,6 +90,49 @@ static void print_query_fileowner(const char *filename, alpm_pkg_t *info) } } +/** Resolve the canonicalized absolute path of a symlink. + * @param path path to resolve + * @param resolved_path destination for the resolved path, will be malloc'd if + * NULL + * @return the resolved path + */ +static char *lrealpath(const char *path, char *resolved_path) +{ + const char *bname = mbasename(path); + char *rpath = NULL, *dname = NULL; + int success = 0; + + if(strcmp(path, ".") == 0 || strcmp(path, "..") == 0) { + /* the entire path needs to be resolved */ + return realpath(path, resolved_path); + } + + if(!(dname = mdirname(path))) { + goto cleanup; + } + if(!(rpath = realpath(dname, NULL))) { + goto cleanup; + } + if(!resolved_path) { + if(!(resolved_path = malloc(strlen(rpath) + strlen(bname) + 2))) { + goto cleanup; + } + } + + strcpy(resolved_path, rpath); + if(resolved_path[strlen(resolved_path) - 1] != '/') { + strcat(resolved_path, "/"); + } + strcat(resolved_path, bname); + success = 1; + +cleanup: + free(dname); + free(rpath); + + return (success ? resolved_path : NULL); +} + static int query_fileowner(alpm_list_t *targets) { int ret = 0; @@ -133,11 +176,11 @@ static int query_fileowner(alpm_list_t *targets) packages = alpm_db_get_pkgcache(db_local); for(t = targets; t; t = alpm_list_next(t)) { - char *filename = NULL, *dname = NULL, *rpath = NULL; - const char *bname; + char *filename = NULL; + char rpath[PATH_MAX], *rel_path; struct stat buf; alpm_list_t *i; - size_t len, is_dir, bname_len, pbname_len; + size_t len, is_dir; unsigned int found = 0; if((filename = strdup(t->data)) == NULL) { @@ -165,82 +208,32 @@ static int query_fileowner(alpm_list_t *targets) } } - is_dir = S_ISDIR(buf.st_mode) ? 1 : 0; - if(is_dir) { - /* the entire filename is safe to resolve if we know it points to a dir, - * and it's necessary in case it ends in . or .. */ - dname = realpath(filename, NULL); - bname = mbasename(dname); - rpath = mdirname(dname); - } else { - bname = mbasename(filename); - dname = mdirname(filename); - rpath = realpath(dname, NULL); - } - - bname_len = strlen(bname); - pbname_len = bname_len + is_dir; - - if(!dname || !rpath) { + if(!lrealpath(filename, rpath)) { pm_printf(ALPM_LOG_ERROR, _("cannot determine real path for '%s': %s\n"), filename, strerror(errno)); goto targcleanup; } - for(i = packages; i && (!found || is_dir); i = alpm_list_next(i)) { - alpm_pkg_t *info = i->data; - alpm_filelist_t *filelist = alpm_pkg_get_files(info); - size_t j; - - for(j = 0; j < filelist->count; j++) { - const alpm_file_t *file = filelist->files + j; - char *ppath; - const char *pkgfile = file->name; - size_t pkgfile_len = strlen(pkgfile); - - /* make sure pkgfile and target are of the same type */ - if(is_dir != (pkgfile[pkgfile_len - 1] == '/')) { - continue; - } - - /* make sure pkgfile is long enough */ - if(pkgfile_len < pbname_len) { - continue; - } - - /* make sure pbname_len takes us to the start of a path component */ - if(pbname_len != pkgfile_len && pkgfile[pkgfile_len - pbname_len - 1] != '/') { - continue; - } + if(strncmp(rpath, path, rootlen) != 0) { + /* file is outside root, we know nothing can own it */ + pm_printf(ALPM_LOG_ERROR, _("No package owns %s\n"), filename); + goto targcleanup; + } - /* compare basename with bname */ - if(strncmp(pkgfile + pkgfile_len - pbname_len, bname, bname_len) != 0) { - continue; - } + rel_path = rpath + rootlen; - /* concatenate our dirname and the root path */ - if(rootlen + 1 + pkgfile_len - pbname_len > PATH_MAX) { - path[rootlen] = '\0'; /* reset path for error message */ - pm_printf(ALPM_LOG_ERROR, _("path too long: %s%s\n"), path, pkgfile); - continue; - } - strncpy(path + rootlen, pkgfile, pkgfile_len - pbname_len); - path[rootlen + pkgfile_len - pbname_len] = '\0'; - - ppath = realpath(path, NULL); - if(!ppath) { - pm_printf(ALPM_LOG_ERROR, _("cannot determine real path for '%s': %s\n"), - path, strerror(errno)); - continue; - } + if((is_dir = S_ISDIR(buf.st_mode))) { + size_t rlen = strlen(rpath); + if(rlen + 2 >= PATH_MAX) { + pm_printf(ALPM_LOG_ERROR, _("path too long: %s/\n"), rpath); + } + strcat(rpath + rlen, "/"); + } - if(strcmp(ppath, rpath) == 0) { - print_query_fileowner(filename, info); - found = 1; - free(ppath); - break; - } - free(ppath); + for(i = packages; i && (!found || is_dir); i = alpm_list_next(i)) { + if(alpm_filelist_contains(alpm_pkg_get_files(i->data), rel_path)) { + print_query_fileowner(rpath, i->data); + found = 1; } } if(!found) { @@ -252,8 +245,6 @@ targcleanup: ret++; } free(filename); - free(rpath); - free(dname); } return ret; -- 1.8.2.2
Packages removed due to conflicts are always removed at the beginning of the transaction and as such can be included in the check for whether all owners of a directory will be removed in a transaction. Installed versions of packages being upgraded, other than the one with the conflict, cannot be used because our transaction ordering is not intelligent enough to ensure that they are removed prior to the installation of the conflicted package. Also, return false from dir_belongsto_pkgs on errors. Previously, we simply continued which could return true even if we were unable to actually establish that the package owned the entire tree. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/conflict.c | 133 +++++++++++++++++------------------ test/pacman/tests/fileconflict030.py | 17 +++++ 2 files changed, 82 insertions(+), 68 deletions(-) create mode 100644 test/pacman/tests/fileconflict030.py diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 08c1cbe..35ab6e8 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -299,94 +299,73 @@ void _alpm_fileconflict_free(alpm_fileconflict_t *conflict) } /** - * @brief Recursively checks if a package owns all subdirectories and files in - * a directory. + * @brief Recursively checks if a set of packages own all subdirectories and + * files in a directory. * * @param handle the context handle * @param dirpath path of the directory to check - * @param pkg package being checked against + * @param pkgs packages being checked against * - * @return 1 if a package owns all subdirectories and files or a directory - * cannot be opened, 0 otherwise + * @return 1 if a package owns all subdirectories and files, 0 otherwise */ -static int dir_belongsto_pkg(alpm_handle_t *handle, const char *dirpath, - alpm_pkg_t *pkg) +static int dir_belongsto_pkgs(alpm_handle_t *handle, const char *dirpath, + alpm_list_t *pkgs) { - alpm_list_t *i; - struct stat sbuf; - char path[PATH_MAX]; - char abspath[PATH_MAX]; + char path[PATH_MAX], full_path[PATH_MAX]; DIR *dir; struct dirent *ent = NULL; - const char *root = handle->root; - - /* check directory is actually in package - used for subdirectory checks */ - if(!alpm_filelist_contains(alpm_pkg_get_files(pkg), dirpath)) { - _alpm_log(handle, ALPM_LOG_DEBUG, - "directory %s not in package %s\n", dirpath, pkg->name); - return 0; - } - - /* TODO: this is an overly strict check but currently pacman will not - * overwrite a directory with a file (case 10/11 in add.c). Adjusting that - * is not simple as even if the directory is being unowned by a conflicting - * package, pacman does not sort this to ensure all required directory - * "removals" happen before installation of file/symlink */ - - /* check that no other _installed_ package owns the directory */ - for(i = _alpm_db_get_pkgcache(handle->db_local); i; i = i->next) { - if(pkg == i->data) { - continue; - } - - if(alpm_filelist_contains(alpm_pkg_get_files(i->data), dirpath)) { - _alpm_log(handle, ALPM_LOG_DEBUG, - "file %s also in package %s\n", dirpath, - ((alpm_pkg_t*)i->data)->name); - return 0; - } - } - /* check all files in directory are owned by the package */ - snprintf(abspath, PATH_MAX, "%s%s", root, dirpath); - dir = opendir(abspath); + snprintf(full_path, PATH_MAX, "%s%s", handle->root, dirpath); + dir = opendir(full_path); if(dir == NULL) { - return 1; + return 0; } while((ent = readdir(dir)) != NULL) { const char *name = ent->d_name; + int owned = 0; + alpm_list_t *i; + struct stat sbuf; if(strcmp(name, ".") == 0 || strcmp(name, "..") == 0) { continue; } + snprintf(path, PATH_MAX, "%s%s", dirpath, name); - snprintf(abspath, PATH_MAX, "%s%s", root, path); - if(stat(abspath, &sbuf) != 0) { - continue; - } - if(S_ISDIR(sbuf.st_mode)) { - if(dir_belongsto_pkg(handle, path, pkg)) { - continue; - } else { - closedir(dir); - return 0; - } - } else { - if(alpm_filelist_contains(alpm_pkg_get_files(pkg), path)) { - continue; - } else { - closedir(dir); - _alpm_log(handle, ALPM_LOG_DEBUG, - "unowned file %s found in directory\n", path); - return 0; + snprintf(full_path, PATH_MAX, "%s%s", handle->root, path); + + for(i = pkgs; i && !owned; i = i->next) { + if(alpm_filelist_contains(alpm_pkg_get_files(i->data), path)) { + owned = 1; } } + + if(owned && stat(full_path, &sbuf) != 0 && S_ISDIR(sbuf.st_mode)) { + owned = dir_belongsto_pkgs(handle, path, pkgs); + } + + if(!owned) { + closedir(dir); + _alpm_log(handle, ALPM_LOG_DEBUG, + "unowned file %s found in directory\n", path); + return 0; + } } closedir(dir); return 1; } +static alpm_list_t *alpm_db_find_file_owners(alpm_db_t* db, const char *path) +{ + alpm_list_t *i, *owners = NULL; + for(i = alpm_db_get_pkgcache(db); i; i = i->next) { + if(alpm_filelist_contains(alpm_pkg_get_files(i->data), path)) { + owners = alpm_list_add(owners, i->data); + } + } + return owners; +} + /** * @brief Find file conflicts that may occur during the transaction. * @@ -571,14 +550,32 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, } /* check if all files of the dir belong to the installed pkg */ - if(!resolved_conflict && S_ISDIR(lsbuf.st_mode) && dbpkg) { + if(!resolved_conflict && S_ISDIR(lsbuf.st_mode)) { + alpm_list_t *owners; char *dir = malloc(strlen(relative_path) + 2); sprintf(dir, "%s/", relative_path); - if(alpm_filelist_contains(alpm_pkg_get_files(dbpkg), dir)) { - _alpm_log(handle, ALPM_LOG_DEBUG, - "checking if all files in %s belong to %s\n", - dir, dbpkg->name); - resolved_conflict = dir_belongsto_pkg(handle, dir, dbpkg); + + owners = alpm_db_find_file_owners(handle->db_local, dir); + if(owners) { + alpm_list_t *pkgs = NULL, *diff; + + if(dbpkg) { + pkgs = alpm_list_add(pkgs, dbpkg); + } + pkgs = alpm_list_join(pkgs, alpm_list_copy(rem)); + if((diff = alpm_list_diff(owners, pkgs, _alpm_pkg_cmp))) { + /* dir is owned by files we aren't removing */ + /* TODO: with better commit ordering, we may be able to check + * against upgrades as well */ + alpm_list_free(diff); + } else { + _alpm_log(handle, ALPM_LOG_DEBUG, + "checking if all files in %s belong to removed packages\n", + dir); + resolved_conflict = dir_belongsto_pkgs(handle, dir, owners); + } + alpm_list_free(pkgs); + alpm_list_free(owners); } free(dir); } diff --git a/test/pacman/tests/fileconflict030.py b/test/pacman/tests/fileconflict030.py new file mode 100644 index 0000000..1de7781 --- /dev/null +++ b/test/pacman/tests/fileconflict030.py @@ -0,0 +1,17 @@ +self.description = "Dir->file transition filesystem conflict resolved by removal" + +lp1 = pmpkg("foo") +lp1.files = ["foo/"] +self.addpkg2db("local", lp1) + +sp1 = pmpkg("bar") +sp1.conflicts = ["foo"] +sp1.files = ["foo"] +self.addpkg2db("sync", sp1) + +self.args = "-S %s --ask=4" % sp1.name + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_EXIST=bar") +self.addrule("!PKG_EXIST=foo") +self.addrule("FILE_EXIST=foo") -- 1.8.2.1
On 27/04/13 10:00, Andrew Gregory wrote:
Two weeks with no objections, so here is the full patchset. Several of the tests should be redundant now, but I left them in with updated rules anyway, just in case.
Thanks - patches look good to me. I made a couple of comments on them. What we also need to figure out is the upgrade path here. There are packages in the Arch repos with files in /lib/... relying on the symlink at the moment. They will create conflicts after these patches (which is fine), but we need to make sure they will upgrade fine to the fixed versions. (They might already - but needs tested.) @Dan, @Dave (or anyone else...): Do you intend to comment on this proposal? Even an "ack" would be appreciated here. For reference: https://mailman.archlinux.org/pipermail/pacman-dev/2013-April/017001.html
Also available on my no-symlinks branch for those interested.
Andrew Gregory (8): update tests for symlink support removal alpm_filelist: remove resolved_path conflict.c: do not ignore symlink<->dir conflicts conflict.c: use real path for filesystem checks extract_single_file: consolidate symlink cases unlink_file: treat symlinks like normal files query_fileowner: remove symlink support improve dir->file transition conflict resolution
lib/libalpm/add.c | 33 ++--- lib/libalpm/alpm.h | 3 +- lib/libalpm/conflict.c | 192 +++++++++++++----------------- lib/libalpm/filelist.c | 225 ++--------------------------------- lib/libalpm/filelist.h | 4 - lib/libalpm/package.c | 22 +--- lib/libalpm/remove.c | 11 +- src/common/util-common.c | 43 +++++++ src/common/util-common.h | 1 + src/pacman/query.c | 90 +++----------- test/pacman/tests/fileconflict007.py | 1 + test/pacman/tests/fileconflict013.py | 5 +- test/pacman/tests/fileconflict022.py | 1 - test/pacman/tests/fileconflict023.py | 7 +- test/pacman/tests/fileconflict025.py | 8 +- test/pacman/tests/fileconflict030.py | 17 +++ test/pacman/tests/symlink001.py | 9 +- test/pacman/tests/sync700.py | 12 +- test/pacman/tests/sync701.py | 11 +- test/pacman/tests/sync702.py | 11 +- 20 files changed, 217 insertions(+), 489 deletions(-) create mode 100644 test/pacman/tests/fileconflict030.py
On Mon, Apr 29, 2013 at 12:41:50AM +1000, Allan McRae wrote:
On 27/04/13 10:00, Andrew Gregory wrote:
Two weeks with no objections, so here is the full patchset. Several of the tests should be redundant now, but I left them in with updated rules anyway, just in case.
Thanks - patches look good to me. I made a couple of comments on them.
What we also need to figure out is the upgrade path here. There are packages in the Arch repos with files in /lib/... relying on the symlink at the moment. They will create conflicts after these patches (which is fine), but we need to make sure they will upgrade fine to the fixed versions. (They might already - but needs tested.)
@Dan, @Dave (or anyone else...): Do you intend to comment on this proposal? Even an "ack" would be appreciated here.
As I mentioned to you on IRC yesterday, I'm still concerned that this is going to break my install pretty hard with /bin and /sbin symlinked to usr/bin. I've had this running for the past year or so without too many problems.
For reference: https://mailman.archlinux.org/pipermail/pacman-dev/2013-April/017001.html
Also available on my no-symlinks branch for those interested.
Andrew Gregory (8): update tests for symlink support removal alpm_filelist: remove resolved_path conflict.c: do not ignore symlink<->dir conflicts conflict.c: use real path for filesystem checks extract_single_file: consolidate symlink cases unlink_file: treat symlinks like normal files query_fileowner: remove symlink support improve dir->file transition conflict resolution
lib/libalpm/add.c | 33 ++--- lib/libalpm/alpm.h | 3 +- lib/libalpm/conflict.c | 192 +++++++++++++----------------- lib/libalpm/filelist.c | 225 ++--------------------------------- lib/libalpm/filelist.h | 4 - lib/libalpm/package.c | 22 +--- lib/libalpm/remove.c | 11 +- src/common/util-common.c | 43 +++++++ src/common/util-common.h | 1 + src/pacman/query.c | 90 +++----------- test/pacman/tests/fileconflict007.py | 1 + test/pacman/tests/fileconflict013.py | 5 +- test/pacman/tests/fileconflict022.py | 1 - test/pacman/tests/fileconflict023.py | 7 +- test/pacman/tests/fileconflict025.py | 8 +- test/pacman/tests/fileconflict030.py | 17 +++ test/pacman/tests/symlink001.py | 9 +- test/pacman/tests/sync700.py | 12 +- test/pacman/tests/sync701.py | 11 +- test/pacman/tests/sync702.py | 11 +- 20 files changed, 217 insertions(+), 489 deletions(-) create mode 100644 test/pacman/tests/fileconflict030.py
On 29/04/13 02:33, Dave Reisner wrote:
On Mon, Apr 29, 2013 at 12:41:50AM +1000, Allan McRae wrote:
On 27/04/13 10:00, Andrew Gregory wrote:
Two weeks with no objections, so here is the full patchset. Several of the tests should be redundant now, but I left them in with updated rules anyway, just in case.
Thanks - patches look good to me. I made a couple of comments on them.
What we also need to figure out is the upgrade path here. There are packages in the Arch repos with files in /lib/... relying on the symlink at the moment. They will create conflicts after these patches (which is fine), but we need to make sure they will upgrade fine to the fixed versions. (They might already - but needs tested.)
@Dan, @Dave (or anyone else...): Do you intend to comment on this proposal? Even an "ack" would be appreciated here.
As I mentioned to you on IRC yesterday, I'm still concerned that this is going to break my install pretty hard with /bin and /sbin symlinked to usr/bin. I've had this running for the past year or so without too many problems.
Is that an objection? Every time there is a package in Arch with a file in /usr/bin and a symlink in /bin your system gets "broken". I'll also point out that you could only do this for that period of time because you were using the git version of pacman. Anyway, I'm guessing there are two problems here: 1) Your local database does not match what is on your filesystem. As our conflict checking assumes the local db to be right, this will probably miss conflicts. So we need to correct the local db for anyone who is currently using this "feature". This falls into the category of figuring out an upgrade path. 2) When the local db is right, programs with files in /bin will cause conflicts until Arch officially does that set up. That I do not consider a pacman problem and you will just need to add them to your rebuild list for this setup. I'd say this is not a pacman issue. Allan
On Mon, Apr 29, 2013 at 10:32:49AM +1000, Allan McRae wrote:
On 29/04/13 02:33, Dave Reisner wrote:
On Mon, Apr 29, 2013 at 12:41:50AM +1000, Allan McRae wrote:
On 27/04/13 10:00, Andrew Gregory wrote:
Two weeks with no objections, so here is the full patchset. Several of the tests should be redundant now, but I left them in with updated rules anyway, just in case.
Thanks - patches look good to me. I made a couple of comments on them.
What we also need to figure out is the upgrade path here. There are packages in the Arch repos with files in /lib/... relying on the symlink at the moment. They will create conflicts after these patches (which is fine), but we need to make sure they will upgrade fine to the fixed versions. (They might already - but needs tested.)
@Dan, @Dave (or anyone else...): Do you intend to comment on this proposal? Even an "ack" would be appreciated here.
As I mentioned to you on IRC yesterday, I'm still concerned that this is going to break my install pretty hard with /bin and /sbin symlinked to usr/bin. I've had this running for the past year or so without too many problems.
Is that an objection? Every time there is a package in Arch with a file in /usr/bin and a symlink in /bin your system gets "broken". I'll also point out that you could only do this for that period of time because you were using the git version of pacman.
Ok, that's fair. I suppose it's not really an objection -- I realize that I'm the unique snowflake here and simplifying the code like this is preferrable.
Anyway, I'm guessing there are two problems here:
1) Your local database does not match what is on your filesystem. As our conflict checking assumes the local db to be right, this will probably miss conflicts. So we need to correct the local db for anyone who is currently using this "feature". This falls into the category of figuring out an upgrade path.
2) When the local db is right, programs with files in /bin will cause conflicts until Arch officially does that set up. That I do not consider a pacman problem and you will just need to add them to your rebuild list for this setup. I'd say this is not a pacman issue.
Yep, I agree with this. So, +1 from me overall. d
On 27/04/13 10:00, Andrew Gregory wrote:
Two weeks with no objections, so here is the full patchset. Several of the tests should be redundant now, but I left them in with updated rules anyway, just in case.
Also available on my no-symlinks branch for those interested.
Andrew Gregory (8): update tests for symlink support removal alpm_filelist: remove resolved_path conflict.c: do not ignore symlink<->dir conflicts conflict.c: use real path for filesystem checks extract_single_file: consolidate symlink cases unlink_file: treat symlinks like normal files query_fileowner: remove symlink support improve dir->file transition conflict resolution
Finally got through the last of these I had in my review queue. As it stands, you symlinks branch is good to go. However, there are two things I am considering before merging this: 1) Arch will finish the /usr merge in the near future (weeks, not months). As a reasonable proportion of people running pacman-git have done silly things like symlink /bin -> usr/bin already and thus need the old behaviour, I think we should merge after the transition is done. (One person is a reasonable portion!). 2) We need to figure out how to update peoples dbs where they have paths that assume directories which are really symlinks. Attached is a really bad script to list packages with issues. I'm sure this can be improved... The question remains, do we want to encourage a "pacman-db-upgrade" run here, or something else? Unlike the last db-upgrade, we can not detect whether this needs to be run or not - so I don't think that is the way to go. Allan
participants (4)
-
Allan McRae
-
Andrew Gregory
-
Christian Hesse
-
Dave Reisner