Merge ~ahasenack/ubuntu/+source/backuppc:xenial-backuppc-smb-1576187 into ubuntu/+source/backuppc:ubuntu/xenial-devel

[Andreas Hasenack]

This branch applies the fix that upstream chose for #1576187. This is different from what was applied in debian (which is what we have in bionic and later), but also fixes the problem, and in a better way we believe.

While writing up the testing instructions, I realized these could be easily scripted, and as such I'm also adding a simple samba-based DEP8 test to this package. This test actually exercises the present bug, so if it's run with the package without the fix, it will correctly fail.

The same dep8 test can and will be added to cosmic+1 once it opens (see https://bugs.launchpad.net/ubuntu/+source/backuppc/+bug/1677755).

Testing instructions are provided in the bug, but they are the same as the newly added DEP8 test, which you can run manually if you want.

Bileto ticket: https://bileto.ubuntu.com/#/ticket/3487
PPA: sudo add-apt-repository ppa:ci-train-ppa-service/3487

UPDATE: the dep8 tests are stuck in bileto and I don't know why, but they run locally just fine.

[Andreas Hasenack (ahasenack)]

Laney found the logs for the dep8 run for this xenial package:

https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-xenial-ci-train-ppa-service-3487/xenial/amd64/b/backuppc/20181016_230458_9f949@/log.gz

[Christian Ehrhardt  (paelzer)]

Nice data digging to get to the log :-)
+1 on adding the test along the fix.
This would also make it catch other simmilar issues further on with the tool relying on output "a lot"

Minor optional changes inline, but +1 on the MP as is if you want to keep it that way.

review: Approve

[Andreas Hasenack (ahasenack)]

It looks better above indeed, thanks.

[Andreas Hasenack (ahasenack)]

Tagged and uploaded.

[Andreas Hasenack (ahasenack)]

Sorry, I missed the comment about the /public contents.

I wrote this in IRC as a reply:
<andreas> I had it copying /usr/bin earlier, then /sbin, and had an issue with symlinks, that's why i switched to /etc
<andreas> I wondered about touching files, but I wanted more content, something more than just empty files, then I thought about creating some random ones and dd-fill them with garbage, but it started getting over engineered I thought

[Andreas Hasenack (ahasenack)]

Please take another look, I'm now creating a set of random files for the backup test.

[Andreas Hasenack (ahasenack)]

I will squash that change later, and I checked with #ubuntu-release that I can reuse the package's version/release.

[Christian Ehrhardt  (paelzer)]

Totally like it now, there is one further improvement on the file creation that I sugegsted inline.
Up to you to implement it or call it good-as-is - you have my approval either way.

review: Approve

[Christian Ehrhardt  (paelzer)]

+1 on the new file creation - thanks

[Andreas Hasenack (ahasenack)]

New and a bit cleaner run: https://bileto.ubuntu.com/excuses/3487/xenial.html

[Christian Ehrhardt  (paelzer)]

What is going on with armhf?

I wondered if samba doesn't even start fine there, but conf/config.pl:1669 looks as this would be a real basic ping that fails.
Would we need (want) to bump all sorts of timeouts for armhf to have a reliable chance to succeed?

[Andreas Hasenack (ahasenack)]

I'll add the verbose option to see if it helps diagnose the problem.

[Andreas Hasenack (ahasenack)]

I'm gonna need a live armhf system for this I think:
Performing a full backup
+ result=0
+ echo Performing a full backup
+ sudo -u backuppc -H /usr/share/backuppc/bin/BackupPC_dump -v -f localhost
cmdSystemOrEval: about to system -c 1 localhost
cmdSystemOrEval: finished: got output Exec of -c 1 localhost failed

CheckHostAlive: first ping failed (256, )
+ result=1
+ check_status 1 Full backup
+ local status_code=1
+ local msg=Full backup
+ [ 1 -ne 0 ]
+ echo ERROR: Full backup failed, exit status was 1

also just saw a permission problem that I will examine and fix:
...
Domain=[WORKGROUP] OS=[Windows 6.1] Server=[Samba 4.3.11-Ubuntu]
tar:316 tarmode is now full, system, hidden, noreset, quiet
tar:957 NT_STATUS_ACCESS_DENIED opening remote file \tmp.O95gzO
tar:957 NT_STATUS_ACCESS_DENIED opening remote file \tmp.PUgM2b

[Andreas Hasenack (ahasenack)]

Oh, check this out.

Non-arm:
+ sudo -u backuppc -H /usr/share/backuppc/bin/BackupPC_dump -v -f localhost
cmdSystemOrEval: about to system /bin/ping -c 1 localhost

Arm:
+ sudo -u backuppc -H /usr/share/backuppc/bin/BackupPC_dump -v -f localhost
cmdSystemOrEval: about to system -c 1 localhost

What's missing in the arm case? :)

Tracking down the missing ping command.

[Andreas Hasenack (ahasenack)]

Got it, for some reason on the armh system it thought it needed to use ping6, for ipv6. Configuring that for the test case fixed it:

cmdSystemOrEval: about to system /bin/ping6 -c 1 localhost

[Andreas Hasenack (ahasenack)]

This now contains all the latest changes.

Updated bileto ticket: https://bileto.ubuntu.com/#/ticket/3487

[Christian Ehrhardt  (paelzer)]

Thanks for sorting out the arm issue. I like this. As usual also remember to submit the test to Debian. But on the SRU which would make it check future regressions it really is nice.

[Andreas Hasenack (ahasenack)]

Tagged and uploaded.

[Andreas Hasenack (ahasenack)]

Submitted to Debian at https://salsa.debian.org/debian/backuppc/merge_requests/1

That git repo seems active, so I don't think I need to file a bug to get their attention.

For Debian, I had to add sudo to the dependency list in debian/tests/control. It's a no-op for ubuntu, and we can adopt that too if they merge.

diff --git a/debian/changelog b/debian/changelog
index 22544b2..57c9e13 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+backuppc (3.3.1-2ubuntu3.4) xenial; urgency=medium
+
+  * d/rules, d/p/smb-compat-fix.patch: cope with changes in newer smbclient.
+    Thanks to Maksym Schipka <[e-mail]> (LP: #1576187)
+  * d/t/{control,smb-backup}: simple smb-based DEP8 test (LP: #1677755)
+
+ -- Andreas Hasenack <[e-mail]>  Tue, 16 Oct 2018 17:58:38 -0300
+
 backuppc (3.3.1-2ubuntu3.3) xenial; urgency=medium
 
   * Fix debian/rules so the patch is actually applied. (LP: #1612600)
diff --git a/debian/patches/smb-compat-fix.patch b/debian/patches/smb-compat-fix.patch
new file mode 100644
index 0000000..b436970
--- /dev/null
+++ b/debian/patches/smb-compat-fix.patch
@@ -0,0 +1,66 @@
+From d7a8403b537ed0068e862abc20065e98209527b7 Mon Sep 17 00:00:00 2001
+From: Maksym Schipka <[e-mail]>
+Date: Sun, 14 Aug 2016 09:24:52 +0100
+Subject: [PATCH] Fixing compatibility with Samba 4.3
+Origin: https://github.com/backuppc/backuppc/commit/c550528be63c3b66744830e992b908fff3f2e271
+Bug: https://github.com/backuppc/backuppc/issues/21
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/backuppc/+bug/1576187
+Bug-Debian: https://bugzilla.samba.org/show_bug.cgi?id=11642
+Last-Update: 2018-10-16
+
+---
+ bin/BackupPC_dump        |  2 ++
+ lib/BackupPC/Xfer/Smb.pm | 13 ++++++++++++-
+ 2 files changed, 14 insertions(+), 1 deletion(-)
+diff --git a/bin/BackupPC_dump b/bin/BackupPC_dump
+index 9491046..e41cb68 100755
+--- debian/backuppc/usr/share/backuppc/bin/BackupPC_dump
++++ debian/backuppc/usr/share/backuppc/bin/BackupPC_dump
+@@ -873,6 +873,8 @@ for my $shareName ( @$ShareNames ) {
+     # Merge the xfer status (need to accumulate counts)
+     #
+     my $newStat = $xfer->getStats;
++    # MAKSYM 14082016: forcing the right file count if some bytes were transferred; ensures compatibility with at least Samba-4.3
++    $newStat->{fileCnt} = $nFilesTotal if ( $useTar && $newStat->{fileCnt} == 0 && $xfer->getStats->{byteCnt} > 0 );
+     if ( $newStat->{fileCnt} == 0 ) {
+        $noFilesErr ||= "No files dumped for share $shareName";
+     }
+diff --git a/lib/BackupPC/Xfer/Smb.pm b/lib/BackupPC/Xfer/Smb.pm
+index eaf002e..48964cb 100644
+--- debian/backuppc/usr/share/backuppc/lib/BackupPC/Xfer/Smb.pm
++++ debian/backuppc/usr/share/backuppc/lib/BackupPC/Xfer/Smb.pm
+@@ -217,6 +217,8 @@ sub readOutput
+         # This section is highly dependent on the version of smbclient.
+         # If you upgrade Samba, make sure that these regexp are still valid.
+         #
++        # MAKSYM 14082016: The next regex will never match on Samba-4.3, as
++        # smbclient doesn't produce output required; keeping it for older Sambas
+         if ( /^\s*(-?\d+) \(\s*\d+[.,]\d kb\/s\) (.*)$/ ) {
+             my $sambaFileSize = $1;
+             my $pcFileName    = $2;
+@@ -230,8 +232,15 @@ sub readOutput
+             $t->{byteCnt} += $2;
+             $t->{fileCnt}++;
+             $t->{XferLOG}->write(\"$_\n") if ( $t->{logLevel} >= 1 );
+-        } elsif ( /^\s*tar: dumped \d+ files/ ) {
++        } elsif ( /^\s*tar: dumped (\d+) files/) {
++            # MAKSYM 14082016: Updating file count to the likely number
+             $t->{xferOK} = 1;
++            $t->{fileCnt} = $1;
++            $t->{XferLOG}->write(\"$_\n") if ( $t->{logLevel} >= 0 );
++        } elsif ( /^\s*tar:\d+\s*Total bytes received: (\d+)/) {
++            # MAKSYM 14082016: Updating byte count to the likely number
++            $t->{xferOK} = 1;
++            $t->{byteCnt} = $1;
+             $t->{XferLOG}->write(\"$_\n") if ( $t->{logLevel} >= 0 );
+         } elsif ( /^\s*tar: restored \d+ files/ ) {
+             $t->{xferOK} = 1;
+@@ -270,6 +279,8 @@ sub readOutput
+         } elsif ( /^\s*directory \\/i ) {
+             $t->{XferLOG}->write(\"$_\n") if ( $t->{logLevel} >= 2 );
+         } elsif ( /smb: \\>/
++                || /^\s*tar:\d+/ # MAKSYM 14082016: ignoring 2 more Samba-4.3 specific lines
++                || /^\s*WARNING:/i
+                 || /^\s*added interface/i
+                 || /^\s*tarmode is now/i
+                 || /^\s*Total bytes written/i
diff --git a/debian/rules b/debian/rules
index 051c1ba..7077ff7 100755
--- a/debian/rules
+++ b/debian/rules
@@ -81,6 +81,7 @@ install: build
 	rm -rf debian/backuppc/etc/backuppc/config.pl
 	patch --no-backup-if-mismatch -p0 < debian/patches/escape-braces-in-perl-regex.diff
 	patch --no-backup-if-mismatch -p0 < debian/patches/setuid.patch
+	patch --no-backup-if-mismatch -p0 < debian/patches/smb-compat-fix.patch
 
 # Build architecture-independent files here.
 binary-indep: build install
diff --git a/debian/tests/control b/debian/tests/control
new file mode 100644
index 0000000..c07f175
--- /dev/null
+++ b/debian/tests/control
@@ -0,0 +1,3 @@
+Tests: smb-backup
+Depends: @, samba, smbclient
+Restrictions: isolation-container, allow-stderr, needs-root
diff --git a/debian/tests/smb-backup b/debian/tests/smb-backup
new file mode 100644
index 0000000..7ec3f21
--- /dev/null
+++ b/debian/tests/smb-backup
@@ -0,0 +1,89 @@
+#!/bin/sh
+
+set -ex
+
+check_status() {
+    local status_code=$1
+    local msg="${2:-task}"
+
+    if [ "$status_code" -ne "0" ]; then
+        echo "ERROR: $msg failed, exit status was $status_code"
+        exit "$status_code"
+    else
+        echo "OK: $msg succeeded"
+    fi
+}
+
+populate_directory() {
+    local target="$1"
+    local tempfile
+
+    # target must exist and be a directory
+    test -d "$target" || exit 1
+
+    for n in $(seq 1 10); do
+        tempfile=$(mktemp "$target/tmp.XXXXXX")
+        # the date command makes each tempfile different
+        # the seq command just produces a lot of output
+        # using a "here" document as to not pollute the set -x
+        # output with large command lines
+        cat > "$tempfile" <<EOF
+$(date +%s%N)
+$(seq 1 81920)
+EOF
+    chmod 0644 "$tempfile"
+    done
+}
+
+add_samba_share() {
+    if ! testparm -s 2>&1 | grep -qE "^\[public\]"; then
+        echo "Adding [public] share"
+        cat >> /etc/samba/smb.conf <<EOFEOF
+[public]
+  path = /public
+  read only = no
+  guest ok = yes
+EOFEOF
+        service smbd reload
+    else
+        echo "No need to add [public] share, continuing."
+    fi
+    echo "Populating share"
+    rm -rf /public
+    mkdir -p /public
+    chmod 1777 /public
+    populate_directory /public
+}
+
+add_localhost_backuppc_config() {
+    cat > /etc/backuppc/localhost.pl <<EOFEOF
+\$Conf{XferMethod} = 'smb';
+\$Conf{XferLogLevel} = 1;
+\$Conf{ClientCharset} = '';
+\$Conf{ClientCharsetLegacy} = 'iso-8859-1';
+\$Conf{SmbShareName} = 'public';
+\$Conf{SmbShareUserName} = '';
+\$Conf{SmbSharePasswd} = '';
+\$Conf{PingPath} = '/bin/ping';
+\$Conf{Ping6Path} = '/bin/ping6';
+EOFEOF
+}
+
+echo "Adding samba share"
+add_samba_share
+
+echo "Configuring backuppc"
+add_localhost_backuppc_config
+
+result=0
+echo "Performing a full backup"
+sudo -u backuppc -H /usr/share/backuppc/bin/BackupPC_dump -v -f localhost || result=$?
+check_status $result "Full backup"
+
+result=0
+echo "Changing share content and performing an incremental backup"
+populate_directory /public
+sudo -u backuppc -H /usr/share/backuppc/bin/BackupPC_dump -v -i localhost || result=$?
+check_status $result "Incremental backup"
+
+echo "Done."