Bug#618920: debootstrap: needs more robust download error handling

[Colin Watson <cjwatson@debian.org>]

On Sat, Mar 19, 2011 at 11:18:14AM -0400, Michael Gilbert wrote:
> debootstrap's current download error handling isn't very robust.  It
> declares success just for the presence of a downloaded file, which may
> be a partial download, or one for which the checksum doesn't match.
> Eventually those conditions will lead to unhandled failures elsewhere
> within debootstrap.

Thanks for the patch!  I've attached a test proxy which (with various
modifications depending on exactly what you're trying to test) can be
useful to forcibly recreate this kind of bug by making certain downloads
be truncated or fail in whatever other way you like.

I'm particularly disturbed that I've found it possible for debootstrap
to succeed in some cases despite some corrupted downloads.  This should
definitely not be possible, and I agree that we need something along the
lines of your patch.

> It may also be useful to expand a bit on these d-i debootstrap error
> messages: when an error happens, the right answer that the user wants is
> to hit 'go back' twice in a row to start the debootstrap all over again,
> but the dialogs are confusing, and 'continue' seems to be the obvious
> choice, but that will lead to the broken debootstrap continuing to
> completion with various brokenness. Anyway, that maybe should be
> submitted as another bug.

This is http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=283600, I
think.

> So, back to the original issue, I've created a patch that will retry
> downloads whenever anything in the "get" routine fails, which I
> believe is much more robust than the current situation.  Please see
> attached patch.

> --- newhd/source/debootstrap-1.0.28/functions	2011-02-21 19:25:08.000000000 -0500
> +++ /usr/share/debootstrap/functions	2011-03-19 10:58:57.000000000 -0400
> @@ -1,3 +1,5 @@
> +MAXATTEMPTS="10"
> +
>  ############################################################### smallutils
>  
>  smallyes() {
> @@ -241,6 +243,13 @@
>  }
>  
>  get () {
> +	for iters in $(seq 1 $MAXATTEMPTS); do
> +		if single_get "$@"; then break; fi
> +		warning RETRYING "Retrying failed download."
> +	done
> +}
> +
> +single_get () {
>  	# args: from dest 'nocache'
>  	# args: from dest [checksum size] [alt {checksum size type}]
>  	local displayname
> @@ -331,13 +340,6 @@
>  		# http/ftp mirror
>  		if wgetprogress -O "$dest" "$from"; then
>  			return 0
> -		elif [ -s "$dest" ]; then
> -			local iters=0
> -			while [ "$iters" -lt 3 ]; do
> -				warning RETRYING "Retrying failed download of %s" "$from"
> -				if wgetprogress -c -O "$dest" "$from"; then break; fi
> -				iters="$(($iters + 1))"
> -			done
[...]

This isn't quite right, though.  You can tell by the change in the
signature of the RETRYING warning; compare that with
base-installer/debian/bootstrap-base.templates and you'll see that
that's going to confuse d-i because there's one parameter too few for
the warning.  In any case, I think the warning should indicate which
download it's retrying.

This can be fixed by moving the retry loop into the original get
function, inside the 'for a in $order' loop.  That way we know $from
when issuing the RETRYING warning.  A further refinement is to remove
$dest2 on failure, so that it doesn't inadvertently confuse something
else later.

I've committed my improved version, which you're welcome to review:

  http://anonscm.debian.org/gitweb/?p=d-i/debootstrap.git;a=commitdiff;h=733069bb97bdfe3f9c16ca4c9ef58685205eabf3;hp=412608c1fbe28074f56d930242e4269d5588d101

Thanks,

-- 
Colin Watson                                       [cjwatson@debian.org]

#! /usr/bin/perl
use warnings;
use strict;
use HTTP::Proxy;
use HTTP::Proxy::HeaderFilter::simple;
use HTTP::Proxy::BodyFilter::simple;

sub truncate {
    my ($self, $dataref, $message, $protocol, $buffer) = @_;
    $$dataref = '';
    #$$dataref = substr $$dataref, 0, 1;
}

my $proxy = HTTP::Proxy->new(port => 7890, logmask => HTTP::Proxy::STATUS);
$proxy->push_filter(
    path => qr/klibc/,
    mime => undef,
    response => HTTP::Proxy::BodyFilter::simple->new(\&truncate));
$proxy->start;