On Wed, 2012-04-18 at 14:20:56 -0700, Steve Langasek wrote:
> On Wed, Apr 18, 2012 at 02:55:48PM +0100, Colin Watson wrote:
> > Steve, I think I missed the specifics of what you wanted to do with
> > libc6-i386/libc6:i386. Would dpkg-divert exiting non-zero have been a
> > problem here?
>
> Yes, it would. The problem we're trying to solve is that libc6:i386
> currently has a Replaces: on libc6-i386 so that it can take over the PI.
> However, this means that if the user later removes libc6:i386, the PI (now
> owned by this package) is removed, and packages depending on libc6-i386 now
> don't work.
>
> The proposed solution was for libc6:i386 to apply a divert to
> /lib/ld-linux.so.2 in its preinst, to preserve any other packages' copies of
> this file in a different location so that they can be restored on removal.
> However, libc6:i386 is not always the foreign-arch libc, sometimes it's the
> transitively-essential one that everything else depends on. So to do this,
> we *must* be able to tell dpkg-divert "always record the diversion, but move
> the file IFF it belongs to a different package." This is what we expected
> from --rename --package, but we hit this bug.
>
> So the expected behavior is:
>
> - libc6-i386 unpacked, libc6:i386 not yet installed: record diversion,
> move /lib/ld-linux.so.2
This has the problem that it introduces a time window where there's no
interpreter for i386 on the system.
> - libc6-i386 not installed, libc6:i386 not yet installed: record diversion,
> nothing moves
> - libc6-i386 unpacked, libc6:i386 already installed: record diversion,
> nothing moves, next upgrade of libc6-i386 redirects to the diverted
> location
And this one (assuming the Replaces are still in place) that if
libc6:i386 gets removed before libc6-i386 is upgraded then there will
be no interpreter left.
> - libc6-i386 not installed, libc6:i386 already installed: record
> diversion, nothing moves
So in any case regardless of the fix for this particular bug, I don't
think dpkg-divert is the right solution for this specific problem. In
this case I think what would make more sense is for Replaces to not
make packages lose track of owned files, just replace them on disk,
which would solve all instances of this problem. But these new
semantics would need to be carefully considered, probably the safest
would be to not expose the shared owning and just track them
internally, but I think there's already a bug related to this.
> On Wed, Apr 18, 2012 at 04:17:30PM +0200, Guillem Jover wrote:
> > On Wed, 2012-04-18 at 14:42:02 +0200, Guillem Jover wrote:
> > > I don't think it seems like a good idea to let the caller insert this
> > > kind of bogus diversion, dpkg itself will try to do the rename on unpack
> > > which would mess the system up anyway (but I've not though about this long
> > > enough). So it would seem better at first sight to just abort on --add.
>
> > Hmm thinking about it, this does not make much sense. As such diversion
> > is legitimate and would be equivalent to one w/o --rename, what might be
> > questionable is the time it gets inserted, so I guess not doing the
> > rename might be the right thing to do, I'll ponder about it a bit more
> > after lunch.
>
> Right, this should be entirely legitimate. Diverting the PI is an extreme
> case (and one we've had to work around differently since we can't rely on
> dpkg in previous releases to handle this), but it illustrates why it's
> important to be consistent in application of diversions regardless of the
> installed state of the named package.
Sure, I agree on the correctness of this fix because dpkg-divert
should be resilient against any kind of state the system might be
in, but I think I'd still question the correctness of such usage.
Attached a new patch.
thanks,
guillem
diff --git a/src/divertcmd.c b/src/divertcmd.c
index 6d3c8bf..694d8d2 100644
--- a/src/divertcmd.c
+++ b/src/divertcmd.c
@@ -382,6 +382,31 @@ divertdb_write(void)
free(dbname);
}
+static bool
+diversion_is_owned_by_self(struct pkgset *set, struct filenamenode *namenode)
+{
+ struct pkginfo *pkg;
+ struct filepackages_iterator *iter;
+ bool owned = false;
+
+ if (set == NULL)
+ return false;
+
+ for (pkg = &set->pkg; pkg; pkg = pkg->arch_next)
+ ensure_packagefiles_available(pkg);
+
+ iter = filepackages_iter_new(namenode);
+ while ((pkg = filepackages_iter_next(iter))) {
+ if (pkg->set == set) {
+ owned = true;
+ break;
+ }
+ }
+ filepackages_iter_free(iter);
+
+ return owned;
+}
+
static int
diversion_add(const char *const *argv)
{
@@ -468,6 +493,13 @@ diversion_add(const char *const *argv)
printf(_("Adding '%s'\n"), diversion_describe(contest));
if (opt_rename)
opt_rename = check_rename(&file_from, &file_to);
+ /* Check we are not diverting a file owned by the same set. */
+ if (opt_rename && diversion_is_owned_by_self(pkgset, fnn_from)) {
+ if (opt_verbose > 0)
+ printf(_("Ignoring request to rename self owned diverted file '%s'"),
+ filename);
+ opt_rename = false;
+ }
if (!opt_test) {
divertdb_write();
if (opt_rename)
diff --git a/src/t/100_dpkg_divert.t b/src/t/100_dpkg_divert.t
index 6d62fe5..f1e2775 100644
--- a/src/t/100_dpkg_divert.t
+++ b/src/t/100_dpkg_divert.t
@@ -33,11 +33,13 @@ if (! -x "@dd") {
exit(0);
}
-plan tests => 251;
+plan tests => 257;
sub cleanup {
system("rm -rf $tmpdir && mkdir -p $testdir");
- system("mkdir -p $admindir/updates && touch $admindir/status");
+ system("mkdir -p $admindir/updates");
+ system("rm -f $admindir/status && touch $admindir/status");
+ system("rm -rf $admindir/info && mkdir -p $admindir/info");
}
sub install_diversions {
@@ -47,6 +49,27 @@ sub install_diversions {
close(O);
}
+sub install_filelist {
+ my ($pkg, $arch, @files) = @_;
+ open(L, "> $admindir/info/$pkg.list");
+ for my $file (@files) {
+ print L "$file\n";
+ }
+ close(L);
+ # Only installed packages have their files list considered.
+ open(S, ">> $admindir/status");
+ print S <<EOF;
+Package: $pkg
+Status: install ok installed
+Version: 0
+Architecture: $arch
+Maintainer: dummy
+Description: dummy
+
+EOF
+ close(S);
+}
+
sub call {
my ($prog, $args, %opts) = @_;
@@ -392,6 +415,24 @@ EOF
cleanup();
+note("Adding diversion of file owned by --package");
+
+install_filelist("coreutils", "i386", "$testdir/foo");
+install_diversions('');
+system("touch $testdir/foo");
+
+call_divert(['--quiet', '--rename', '--add', '--package', 'coreutils', "$testdir/foo"],
+ expect_stderr => '', expect_stdout => '');
+ok(-e "$testdir/foo", "foo not renamed");
+ok(!-e "$testdir/foo.distrib", "foo renamed");
+diversions_eq(<<EOF);
+$testdir/foo
+$testdir/foo.distrib
+coreutils
+EOF
+
+cleanup();
+
note("Remove diversions");
install_diversions('');
