Merge lp:~neil-aldur/ubuntu/precise/iptables-persistent/fix-for-967018-905517 into lp:ubuntu/precise/iptables-persistent

[Neil Wilson]

* debian/rules: add missing '.' (LP: #920055)
* debian/postinst: make sure new target file exists so that
  'dpkg-maintscript-helper' works properly (LP: #967018)

Fix a couple of upgrade and installation faults with the maintainer scripts

[Chow Loong Jin (hyperair)]

The debian/rules change looks okay, but the postinst change looks wrong.

Specifically, I think it should be:
if [ ! -e /etc/iptables/rules.v4 ]; then
    touch /etc/iptables/rules.v4
    dpkg-maintscript-helper mv_conffile \
 /etc/iptables/rules /etc/iptables/rules.v4 0.0.20101230 -- "$@"
fi

i.e. you need to check for the presence of rules.v4, rather than rules.

Apart from that, I don't see anything wrong, and will happily sponsor this patch.

review: Needs Fixing

[Neil Wilson (neil-aldur)]

It needs to be both. You don't want to execute the mv_conffile unless both the files exist and you don't want to create a rules.v4 unless the old one already exists and you really need to do a mv_conffile.

You want to avoid the blank rules.v4 file which would cause the init script to try and do an unnecessary firewall load on a blank file.

It's a bit of an abuse of the mv_conffile facility really.

So I'm thinking

if [ -e /etc/iptables/rules ]; then
    [ -e /etc/iptables/rules.v4 ] || touch /etc/iptables/rules.v4
    dpkg-maintscript-helper mv_conffile \
        /etc/iptables/rules /etc/iptables/rules.v4 0.0.20101230 -- "$@"
fi

Which should make sure that the old file is only moved if the target isn't newer.

[Chow Loong Jin (hyperair)]

Ah yes, you're right. Could you apply that change?

[Neil Wilson (neil-aldur)]

Applied

review: Needs Resubmitting

[Chow Loong Jin (hyperair)]

Okay, I've just built and tested upgrading from 0.0.20101230 (the natty package). I noticed that if both /etc/iptables/rules and /etc/iptables/rules.v4 were present, rules.v4 would be clobbered by rules. A backup would be made in rules.v4.dpkg-new, of course, but I'm not sure that's a good thing.

How about the following?

if [ -e /etc/iptables/rules -a ! -e /etc/iptables/rules.v4 ]; then
    touch /etc/iptables/rules.v4
    dpkg-maintscript-helper mv_conffile \
        /etc/iptables/rules /etc/iptables/rules.v4 0.0.20101230 -- "$@"
fi

This should prevent rules.v4 from getting clobbered if the user had, by some freak accident (perhaps an upgrade and a downgrade, and an upgrade again) both rules and rules.v4 where rules.v4 was the relevant file.

[Chow Loong Jin (hyperair)]

Also, you need a trailing space before ].

[Neil Wilson (neil-aldur)]

On a separate note do we think that this is really a fault in dpkg-maintscript-helper we're trying to work around?

if

mv -f "$NEWCONFFILE" "$NEWCONFFILE".dpkg-new"

was replaced with

[ -e "$NEWCONFFILE" ] && mv -f "$NEWCONFFILE" "$NEWCONFFILE".dpkg-new"

It would solve the problem and we could just call:

dpkg-maintscript-helper mv_conffile \
 /etc/iptables/rules /etc/iptables/rules.v4 0.0.20101230 -- "$@"

without any guard at all.

[Neil Wilson (neil-aldur)]

OK, gone for a slightly different approach now.

The mv_conffile only applies to natty and older packages - which have no rules.v4 file at all. So we don't have to worry about whether it gets clobbered or not.

So if the old file exists I create a new rules.v4 file if that doesn't already exist (bit belt and braces but safest).

Then after the move I clear up empty dpkg-new files - which has the added advantage of clearing up after the empty files left by 'oneiric' upgrades from 'natty' at the same time.

I've given this a test on a natty to oneiric to precise upgrade, and the lucid to precise upgrade (and a natty to precise backport upgrade) and it appears to do what I'd expect from each starting state.

[Chow Loong Jin (hyperair)]

I'm not sure [ ! -e /etc/iptables/rules -o -e /etc/iptables/rules.v4 ] is correct. Why do we need to touch /etc/iptables/rules.v4 if it already exists?

Also, I don't really like the idea of automatically removing the rules.v4.dpkg-new file, even if it is empty. I feel that the .dpkg-new and .dpkg-old files are the domain of the administrator, and should not be touched by scripts.

[Neil Wilson (neil-aldur)]

On 19 April 2012 12:53, Chow Loong Jin <email address hidden> wrote:
> I'm not sure [ ! -e /etc/iptables/rules -o -e /etc/iptables/rules.v4 ] is correct. Why do we need to touch /etc/iptables/rules.v4 if it already exists?
>
> Also, I don't really like the idea of automatically removing the rules.v4.dpkg-new file, even if it is empty. I feel that the .dpkg-new and .dpkg-old files are the domain of the administrator, and should not be touched by scripts.
> --
> https://code.launchpad.net/~neil-aldur/ubuntu/precise/iptables-persistent/fix-for-967018-905517/+merge/100652
> You are the owner of lp:~neil-aldur/ubuntu/precise/iptables-persistent/fix-for-967018-905517.

Read the code carefully. It's an || after the test. That's the
condition for *not* touching the file.

And as I said, I've tested this, and it works.

Debian disagree with you. The empty dpkg-new files and the alterations
it put in place in this package is what caused all the fun in the
first place.

See http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=612278

--
Neil Wilson

[lp:~neil-aldur/ubuntu/precise/iptables-persistent/fix-for-967018-905517]

14. By Neil Wilson on 2012-04-19

Stop using 'dpkg-maintscript-helper' for configuration files (LP: #967018)

[Neil Wilson (neil-aldur)]

Based on the discussion around the maintainer script it appears that it really isn't designed to handle 'configuration files'.

So this version strips the lot and goes for the simple solution.

[Chow Loong Jin (hyperair)]

On 19/04/2012 23:26, Neil Wilson wrote:
> Based on the discussion around the maintainer script it appears that it
> really isn't designed to handle 'configuration files'.
>
> So this version strips the lot and goes for the simple solution.

I think was better one commit ago. See http://wiki.debian.org/DpkgConffileHandling.

--
Kind regards,
Loong Jin

[Neil Wilson (neil-aldur)]

That's the point.

It's a *configuration file*, not a conffile.

http://www.debian.org/doc/debian-policy/ch-files.html#s-config-files

[Neil Wilson (neil-aldur)]

See https://bugs.launchpad.net/ubuntu/+source/dpkg/+bug/985508/comments/6

[Chow Loong Jin (hyperair)]

On 19/04/2012 23:41, Chow Loong Jin wrote:
> On 19/04/2012 23:26, Neil Wilson wrote:
>> Based on the discussion around the maintainer script it appears that it
>> really isn't designed to handle 'configuration files'.
>>
>> So this version strips the lot and goes for the simple solution.
>
> I think was better one commit ago. See http://wiki.debian.org/DpkgConffileHandling.
>
Okay, I just found https://bugs.launchpad.net/ubuntu/+source/dpkg/+bug/985508.
It's true that it's not a conffile, but on the other hand, I think it should be
treated somewhat like a conffile, at least with respect to the suffix used.

Either way, at this point, I think the situation gets a little messy and you
should probably discuss this with the maintainer of this package on the Debian
side of things: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=665813

--
Kind regards,
Loong Jin

[lp:~neil-aldur/ubuntu/precise/iptables-persistent/fix-for-967018-905517]

15. By Neil Wilson on 2012-04-19

Change backup name to be consistent with dpkg-mainthelper-script

Check is unlikely to be called in normal usage anyway.

[Chow Loong Jin (hyperair)]

Looks good, thanks!

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2012-02-13 16:13:28 +0000
+++ debian/changelog	2012-04-19 16:13:19 +0000
@@ -1,3 +1,10 @@
+iptables-persistent (0.5.3ubuntu2) precise; urgency=low
+
+  * debian/rules: add missing '.' (LP: #920055)
+  * Stop using 'dpkg-maintscript-helper' for configuration files (LP: #967018)
+
+ -- Neil Wilson <[e-mail]>  Tue, 03 Apr 2012 17:57:01 +0100
+
 iptables-persistent (0.5.3ubuntu1) precise; urgency=low
 
   * Pre-Depend on a sufficient version of dpkg rather than using

=== modified file 'debian/iptables-persistent.postinst'
--- debian/iptables-persistent.postinst	2012-02-13 16:13:28 +0000
+++ debian/iptables-persistent.postinst	2012-04-19 16:13:19 +0000
@@ -5,9 +5,6 @@
 # Source debconf library
 . /usr/share/debconf/confmodule
 
-dpkg-maintscript-helper mv_conffile \
-    /etc/iptables/rules /etc/iptables/rules.v4 0.0.20101230 -- "$@"
-
 case "$1" in
 configure)
     db_get iptables-persistent/autosave_done || true

=== modified file 'debian/iptables-persistent.postrm'
--- debian/iptables-persistent.postrm	2012-02-13 16:13:28 +0000
+++ debian/iptables-persistent.postrm	2012-04-19 16:13:19 +0000
@@ -2,14 +2,11 @@
 
 set -e
 
-dpkg-maintscript-helper mv_conffile \
-     /etc/iptables/rules /etc/iptables/rules.v4 0.0.20101230 -- "$@"
-
 case "$1" in
 purge)
 	rm -rf /etc/iptables/rules \
-		/etc/iptables/rules.v4 \
-		/etc/iptables/rules.v6
+		/etc/iptables/rules.v4* \
+		/etc/iptables/rules.v6*
 ;;
 esac
 

=== modified file 'debian/iptables-persistent.preinst'
--- debian/iptables-persistent.preinst	2012-02-13 16:13:28 +0000
+++ debian/iptables-persistent.preinst	2012-04-19 16:13:19 +0000
@@ -2,7 +2,12 @@
 
 set -e
 
-dpkg-maintscript-helper mv_conffile \
-     /etc/iptables/rules /etc/iptables/rules.v4 0.0.20101230 -- "$@"
+if [ -e /etc/iptables/rules ]; then
+    if [ -e /etc/iptables/rules.v4 ]; then
+        mv -f /etc/iptables/rules /etc/iptables/rules.v4.dpkg-bak
+    else
+    	mv -f /etc/iptables/rules /etc/iptables/rules.v4
+    fi
+fi
 
 #DEBHELPER#

=== modified file 'debian/rules'
--- debian/rules	2011-11-28 23:19:39 +0000
+++ debian/rules	2012-04-19 16:13:19 +0000
@@ -1,7 +1,7 @@
 #!/usr/bin/make -f
 
 override_dh_installinit:
-	dh_installinit -- start 37 2 3 4 5 . stop 37 0 1 6
+	dh_installinit -- start 37 2 3 4 5 . stop 37 0 1 6 .
 
 %:
 	dh $@