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. 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. 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. A later commit ("distinguish checksum mismatches from retrieval errors") does: - if ! preseed_fetch "$location" "$tmp" "$sum"; then + retval=0 + preseed_fetch "$location" "$tmp" "$checksum" || retval=$? + case $retval in + 0) ;; + 2) + log "checksum error \"$location\", expected \"$checksum\"" + error checksum_error "$location" "$checksum" + ;; It looks like all instances of "$checksum" should be "$sum". Another commit ("update auto-install/defaultroot for stretch") bumps the changelog version from 1.73 to 1.76. Huh? 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? [...] > 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. Ben. -- Ben Hutchings The world is coming to an end. Please log off.
signature.asc
Description: This is a digitally signed message part