Ben Hutchings <ben@decadent.org.uk> writes:
> On Sat, 2016-11-05 at 01:03 +0100, Philip Hands wrote:
>> Hi,
>>
>> First, apologies for the timing on this, real-life has eaten all my time
>> of late, and when I had some time to spare the InRelease split bug threw
>> a spanner in the works.
>>
>> Here are some patches that I've been failing to get into d-i for a
>> couple of releases -- it would be really nice to avoid making that 3
>> releases.
>>
>> https://anonscm.debian.org/cgit/d-i/preseed.git/log/?h=pu/preseed-fetch-checksum
>> and
>> https://anonscm.debian.org/cgit/d-i/debian-installer-utils.git/log/?h=pu/preseed-fetch-checksum
>
> I have some review comments.
>
> I think fetch-url should delete the destination file if its checksum is
> incorrect. The destination might be used as a cache location and
> assumed to be valid at a later stage.
Good point, although its not nearly as important in d-i as it might be
elsewhere, since the error is almost certainly going to terminate the
install.
Also, deleting the file means that one loses the chance to diagnose what
happened.
One could do the fetch to a temporary file, and then only move it into
place on success.
> I think the commit to preseed that relies on this ("allow preseed_fetch
> to pass all it's params onto fetch-url") should also add a versioned
> dependency on debian-installer-utils.
Makes sense.
> The commit message could also be improved - this change doesn't
> *allow* passing those parameters; it *does* pass them. Finally, it
> adds a spurious blank line further down debian/changelog.
Well spotted -- seems I was untidy when using merge.
...
> It looks like all instances of "$checksum" should be "$sum".
That would appear to be the case, so it seems that I'd not managed to
cover that code in it's latest incarnation in my tests. Thanks for
that.
> Another commit ("update auto-install/defaultroot for stretch") bumps
> the changelog version from 1.73 to 1.76. Huh?
I seem to be in the habit of adding a -i option to dch. :-/
> The last commit ("add a hook for looking up preseed checksums") does:
>
> +if [ "$LOOKUP_CHECKSUM" = 1 ] ; then
> + # this is yet to be written, so leave room for dropping it into place during preseeding
> + if [ -f /bin/preseed_lookup_checksum ] ; then
> + # the "$@" parameter should be empty, but we might as well pass it in, so that we can pass it back out if need be
> + set -- $(/bin/preseed_lookup_checksum "$src" "$@")
> + fi
> +fi
>
> which seems to mean that when the -C option is used and
> /bin/preseed_lookup_checksum is missing then checksums are not
> verified. Shouldn't preseed_fetch fail in this case?
The thought had flitted through my mind, so thanks for the reminder. I
agree that it should fail. I think I did it that way because I was
being overly paranoid about not breaking existing behaviour at the time.
> [...]
>> P.S. I think this would be better if it were using SHA>=265, but
>> we're
> still on MD5SUM elsewhere in the code, so I guess that will need to
>> wait.
>
> I agree, MD5 should not be relied on for more than error detection.
I presume now is the wrong moment to start changing all that though.
The new incarnation is here:
https://anonscm.debian.org/git/d-i/preseed.git/log/?h=pu/preseed-fetch-checksum2
I'll get on with testing...
Thanks again for the feedback Ben. :-)
Cheers, Phil.
--
|)| Philip Hands [+44 (0)20 8530 9560] HANDS.COM Ltd.
|-| http://www.hands.com/ http://ftp.uk.debian.org/
|(| Hugo-Klemm-Strasse 34, 21075 Hamburg, GERMANY
Attachment:
signature.asc
Description: PGP signature