From 2d6c6c8809c7b4c1a009df48387ba15066fda7e2 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 14 Jan 2018 00:07:20 +0100 Subject: Drop alternative URIs we got a hash-based fail from If we got a file but it produced a hash error, mismatched size or similar we shouldn't fallback to alternative URIs as they likely result in the same error. If we can we should instead use another mirror. We used to be a lot stricter by stopping all trys for this file if we got a non-404 (or a hash-based) failure, but that is too hard as we really want to try other mirrors (if we have them) in the hope that they have the expected and correct files. --- apt-pkg/acquire-item.cc | 112 ++++++++++++++--------------- apt-pkg/acquire-item.h | 20 +++--- apt-pkg/acquire-worker.cc | 84 ++++++++++++---------- test/integration/test-partial-file-support | 2 +- test/integration/test-pdiff-usage | 3 +- 5 files changed, 117 insertions(+), 104 deletions(-) diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index f5986a260..d0b92ec66 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -399,18 +399,23 @@ bool pkgAcqTransactionItem::QueueURI(pkgAcquire::ItemDesc &Item) return false; } // If we got the InRelease file via a mirror, pick all indexes directly from this mirror, too - if (TransactionManager->BaseURI.empty() == false && UsedMirror.empty() && - URI::SiteOnly(Item.URI) != URI::SiteOnly(TransactionManager->BaseURI)) + if (TransactionManager->BaseURI.empty() == false && TransactionManager->UsedMirror.empty() == false && + URI::SiteOnly(Item.URI) != URI::SiteOnly(TransactionManager->BaseURI)) { // this ensures we rewrite only once and only the first step auto const OldBaseURI = Target.Option(IndexTarget::BASE_URI); if (OldBaseURI.empty() == false && APT::String::Startswith(Item.URI, OldBaseURI)) { auto const ExtraPath = Item.URI.substr(OldBaseURI.length()); - Item.URI = flCombine(TransactionManager->BaseURI, ExtraPath); - UsedMirror = TransactionManager->UsedMirror; - if (Item.Description.find(" ") != string::npos) - Item.Description.replace(0, Item.Description.find(" "), UsedMirror); + auto newURI = flCombine(TransactionManager->BaseURI, ExtraPath); + if (IsGoodAlternativeURI(newURI)) + { + PushAlternativeURI(std::string(Item.URI), {}, false); + Item.URI = std::move(newURI); + UsedMirror = TransactionManager->UsedMirror; + if (Item.Description.find(" ") != string::npos) + Item.Description.replace(0, Item.Description.find(" "), UsedMirror); + } } } return pkgAcquire::Item::QueueURI(Item); @@ -686,11 +691,12 @@ class pkgAcquire::Item::Private public: struct AlternateURI { - std::string const URI; + std::string URI; std::unordered_map changefields; AlternateURI(std::string &&u, decltype(changefields) &&cf) : URI(u), changefields(cf) {} }; std::list AlternativeURIs; + std::vector BadAlternativeSites; std::vector PastRedirections; std::unordered_map CustomFields; unsigned int Retries; @@ -749,14 +755,32 @@ bool pkgAcquire::Item::PopAlternativeURI(std::string &NewURI) /*{{{*/ return true; } /*}}}*/ +bool pkgAcquire::Item::IsGoodAlternativeURI(std::string const &AltUri) const/*{{{*/ +{ + return std::find(d->PastRedirections.cbegin(), d->PastRedirections.cend(), AltUri) == d->PastRedirections.cend() && + std::find(d->BadAlternativeSites.cbegin(), d->BadAlternativeSites.cend(), URI::SiteOnly(AltUri)) == d->BadAlternativeSites.cend(); +} + /*}}}*/ void pkgAcquire::Item::PushAlternativeURI(std::string &&NewURI, std::unordered_map &&fields, bool const at_the_back) /*{{{*/ { + if (IsGoodAlternativeURI(NewURI) == false) + return; if (at_the_back) d->AlternativeURIs.emplace_back(std::move(NewURI), std::move(fields)); else d->AlternativeURIs.emplace_front(std::move(NewURI), std::move(fields)); } /*}}}*/ +void pkgAcquire::Item::RemoveAlternativeSite(std::string &&OldSite) /*{{{*/ +{ + d->AlternativeURIs.erase(std::remove_if(d->AlternativeURIs.begin(), d->AlternativeURIs.end(), + [&](decltype(*d->AlternativeURIs.cbegin()) AltUri) { + return URI::SiteOnly(AltUri.URI) == OldSite; + }), + d->AlternativeURIs.end()); + d->BadAlternativeSites.push_back(std::move(OldSite)); +} + /*}}}*/ unsigned int &pkgAcquire::Item::ModifyRetries() /*{{{*/ { return d->Retries; @@ -2231,7 +2255,7 @@ void pkgAcqDiffIndex::QueueOnIMSHit() const /*{{{*/ { // list cleanup needs to know that this file as well as the already // present index is ours, so we create an empty diff to save it for us - new pkgAcqIndexDiffs(Owner, TransactionManager, Target, UsedMirror, Target.URI); + new pkgAcqIndexDiffs(Owner, TransactionManager, Target, Target.URI); } /*}}}*/ static bool RemoveFileForBootstrapLinking(std::string &ErrorText, std::string const &For, std::string const &Boot)/*{{{*/ @@ -2585,7 +2609,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/ /*}}}*/ void pkgAcqDiffIndex::Failed(string const &Message,pkgAcquire::MethodConfig const * const Cnf)/*{{{*/ { - if (CommonFailed(GetDiffIndexURI(Target), GetDiffIndexFileName(Target.Description), Message, Cnf)) + if (CommonFailed(GetDiffIndexURI(Target), Message, Cnf)) return; RenameOnError(PDiffError); @@ -2649,15 +2673,15 @@ void pkgAcqDiffIndex::Done(string const &Message,HashStringList const &Hashes, / } if (pdiff_merge == false) - new pkgAcqIndexDiffs(Owner, TransactionManager, Target, UsedMirror, indexURI, available_patches); + new pkgAcqIndexDiffs(Owner, TransactionManager, Target, indexURI, available_patches); else { diffs = new std::vector(available_patches.size()); for(size_t i = 0; i < available_patches.size(); ++i) (*diffs)[i] = new pkgAcqIndexMergeDiffs(Owner, TransactionManager, - Target, UsedMirror, indexURI, - available_patches[i], - diffs); + Target, indexURI, + available_patches[i], + diffs); } } @@ -2681,13 +2705,13 @@ pkgAcqDiffIndex::~pkgAcqDiffIndex() /* The package diff is added to the queue. one object is constructed * for each diff and the index */ -pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire * const Owner, - pkgAcqMetaClearSig * const TransactionManager, - IndexTarget const &Target, - std::string const &indexUsedMirror, std::string const &indexURI, +pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire *const Owner, + pkgAcqMetaClearSig *const TransactionManager, + IndexTarget const &Target, + std::string const &indexURI, vector const &diffs) - : pkgAcqBaseIndex(Owner, TransactionManager, Target), indexURI(indexURI), - available_patches(diffs) + : pkgAcqBaseIndex(Owner, TransactionManager, Target), indexURI(indexURI), + available_patches(diffs) { DestFile = GetKeepCompressedFileName(GetPartialFileNameFromURI(Target.URI), Target); @@ -2697,12 +2721,6 @@ pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire * const Owner, Description = Target.Description; Desc.ShortDesc = Target.ShortDesc; - UsedMirror = indexUsedMirror; - if (UsedMirror == "DIRECT") - UsedMirror.clear(); - else if (UsedMirror.empty() == false && Description.find(" ") != string::npos) - Description.replace(0, Description.find(" "), UsedMirror); - if(available_patches.empty() == true) { // we are done (yeah!), check hashes against the final file @@ -2870,7 +2888,7 @@ void pkgAcqIndexDiffs::Done(string const &Message, HashStringList const &Hashes, // see if there is more to download if(available_patches.empty() == false) { - new pkgAcqIndexDiffs(Owner, TransactionManager, Target, UsedMirror, indexURI, available_patches); + new pkgAcqIndexDiffs(Owner, TransactionManager, Target, indexURI, available_patches); Finish(); } else { DestFile = PatchedFile; @@ -2896,23 +2914,17 @@ std::string pkgAcqIndexDiffs::Custom600Headers() const /*{{{*/ pkgAcqIndexDiffs::~pkgAcqIndexDiffs() {} // AcqIndexMergeDiffs::AcqIndexMergeDiffs - Constructor /*{{{*/ -pkgAcqIndexMergeDiffs::pkgAcqIndexMergeDiffs(pkgAcquire * const Owner, - pkgAcqMetaClearSig * const TransactionManager, - IndexTarget const &Target, - std::string const &indexUsedMirror, std::string const &indexURI, - DiffInfo const &patch, - std::vector const * const allPatches) - : pkgAcqBaseIndex(Owner, TransactionManager, Target), indexURI(indexURI), - patch(patch), allPatches(allPatches), State(StateFetchDiff) +pkgAcqIndexMergeDiffs::pkgAcqIndexMergeDiffs(pkgAcquire *const Owner, + pkgAcqMetaClearSig *const TransactionManager, + IndexTarget const &Target, + std::string const &indexURI, + DiffInfo const &patch, + std::vector const *const allPatches) + : pkgAcqBaseIndex(Owner, TransactionManager, Target), indexURI(indexURI), + patch(patch), allPatches(allPatches), State(StateFetchDiff) { Debug = _config->FindB("Debug::pkgAcquire::Diffs",false); - Description = Target.Description; - UsedMirror = indexUsedMirror; - if (UsedMirror == "DIRECT") - UsedMirror.clear(); - else if (UsedMirror.empty() == false && Description.find(" ") != string::npos) - Description.replace(0, Description.find(" "), UsedMirror); Desc.Owner = this; Desc.ShortDesc = Target.ShortDesc; @@ -3170,24 +3182,10 @@ string pkgAcqIndex::Custom600Headers() const } /*}}}*/ // AcqIndex::Failed - getting the indexfile failed /*{{{*/ -bool pkgAcqIndex::CommonFailed(std::string const &TargetURI, std::string const TargetDesc, - std::string const &Message, pkgAcquire::MethodConfig const * const Cnf) +bool pkgAcqIndex::CommonFailed(std::string const &TargetURI, + std::string const &Message, pkgAcquire::MethodConfig const *const Cnf) { pkgAcqBaseIndex::Failed(Message,Cnf); - - if (UsedMirror.empty() == false && UsedMirror != "DIRECT" && - LookupTag(Message, "FailReason") == "HttpError404") - { - UsedMirror = "DIRECT"; - if (Desc.URI.find("/by-hash/") != std::string::npos) - CompressionExtensions = "by-hash " + CompressionExtensions; - else - CompressionExtensions = CurrentCompressionExtension + ' ' + CompressionExtensions; - Init(TargetURI, TargetDesc, Desc.ShortDesc); - Status = StatIdle; - return true; - } - // authorisation matches will not be fixed by other compression types if (Status != StatAuthError) { @@ -3202,7 +3200,7 @@ bool pkgAcqIndex::CommonFailed(std::string const &TargetURI, std::string const T } void pkgAcqIndex::Failed(string const &Message,pkgAcquire::MethodConfig const * const Cnf) { - if (CommonFailed(Target.URI, Target.Description, Message, Cnf)) + if (CommonFailed(Target.URI, Message, Cnf)) return; if(Target.IsOptional && GetExpectedHashes().empty() && Stage == STAGE_DOWNLOAD) diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index 46d79df92..814596850 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -246,7 +246,9 @@ class pkgAcquire::Item : public WeakPointable /*{{{*/ APT_HIDDEN std::unordered_map &ModifyCustomFields(); // this isn't the super nicest interface either… APT_HIDDEN bool PopAlternativeURI(std::string &NewURI); + APT_HIDDEN bool IsGoodAlternativeURI(std::string const &AltUri) const; APT_HIDDEN void PushAlternativeURI(std::string &&NewURI, std::unordered_map &&fields, bool const at_the_back); + APT_HIDDEN void RemoveAlternativeSite(std::string &&OldSite); /** \brief A "descriptive" URI-like string. * @@ -690,8 +692,8 @@ class APT_HIDDEN pkgAcqIndex : public pkgAcqBaseIndex protected: APT_HIDDEN void Init(std::string const &URI, std::string const &URIDesc, std::string const &ShortDesc); - APT_HIDDEN bool CommonFailed(std::string const &TargetURI, std::string const TargetDesc, - std::string const &Message, pkgAcquire::MethodConfig const * const Cnf); + APT_HIDDEN bool CommonFailed(std::string const &TargetURI, + std::string const &Message, pkgAcquire::MethodConfig const *const Cnf); }; /*}}}*/ struct APT_HIDDEN DiffInfo { /*{{{*/ @@ -850,10 +852,10 @@ class APT_HIDDEN pkgAcqIndexMergeDiffs : public pkgAcqBaseIndex * \param allPatches contains all related items so that each item can * check if it was the last one to complete the download step */ - pkgAcqIndexMergeDiffs(pkgAcquire * const Owner, pkgAcqMetaClearSig * const TransactionManager, - IndexTarget const &Target, std::string const &indexUsedMirror, + pkgAcqIndexMergeDiffs(pkgAcquire *const Owner, pkgAcqMetaClearSig *const TransactionManager, + IndexTarget const &Target, std::string const &indexURI, DiffInfo const &patch, - std::vector const * const allPatches) APT_NONNULL(2, 3, 8); + std::vector const *const allPatches) APT_NONNULL(2, 3, 7); virtual ~pkgAcqIndexMergeDiffs(); }; /*}}}*/ @@ -957,10 +959,10 @@ class APT_HIDDEN pkgAcqIndexDiffs : public pkgAcqBaseIndex * should be ordered so that each diff appears before any diff * that depends on it. */ - pkgAcqIndexDiffs(pkgAcquire * const Owner, pkgAcqMetaClearSig * const TransactionManager, - IndexTarget const &Target, - std::string const &indexUsedMirror, std::string const &indexURI, - std::vector const &diffs=std::vector()) APT_NONNULL(2, 3); + pkgAcqIndexDiffs(pkgAcquire *const Owner, pkgAcqMetaClearSig *const TransactionManager, + IndexTarget const &Target, + std::string const &indexURI, + std::vector const &diffs = std::vector()) APT_NONNULL(2, 3); virtual ~pkgAcqIndexDiffs(); }; /*}}}*/ diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc index d159ef84f..c2bbf8bed 100644 --- a/apt-pkg/acquire-worker.cc +++ b/apt-pkg/acquire-worker.cc @@ -321,28 +321,35 @@ bool pkgAcquire::Worker::RunMessages() Itm = nullptr; for (auto const &Owner: ItmOwners) { + for (auto alt = AltUris.crbegin(); alt != AltUris.crend(); ++alt) + Owner->PushAlternativeURI(std::string(*alt), {}, false); + pkgAcquire::ItemDesc &desc = Owner->GetItemDesc(); - if (Owner->IsRedirectionLoop(NewURI)) + // for a simplified retry a method might redirect without URI change + // see also IsRedirectionLoop implementation + if (desc.URI != NewURI) { - std::string msg = Message; - msg.append("\nFailReason: RedirectionLoop"); - Owner->Failed(msg, Config); - if (Log != nullptr) - Log->Fail(Owner->GetItemDesc()); - continue; - } + auto newuri = NewURI; + if (Owner->IsGoodAlternativeURI(newuri) == false && Owner->PopAlternativeURI(newuri) == false) + newuri.clear(); + if (newuri.empty() || Owner->IsRedirectionLoop(newuri)) + { + std::string msg = Message; + msg.append("\nFailReason: RedirectionLoop"); + Owner->Failed(msg, Config); + if (Log != nullptr) + Log->Fail(Owner->GetItemDesc()); + continue; + } - if (Log != nullptr) - Log->Done(desc); + if (Log != nullptr) + Log->Done(desc); - ChangeSiteIsMirrorChange(NewURI, desc, Owner); - desc.URI = NewURI; + ChangeSiteIsMirrorChange(NewURI, desc, Owner); + desc.URI = NewURI; + } if (isDoomedItem(Owner) == false) - { - for (auto alt = AltUris.crbegin(); alt != AltUris.crend(); ++alt) - Owner->PushAlternativeURI(std::string(*alt), {}, false); OwnerQ->Owner->Enqueue(desc); - } } break; } @@ -608,28 +615,33 @@ void pkgAcquire::Worker::HandleFailure(std::vector const &It if (isDoomedItem(Owner) == false) OwnerQ->Owner->Enqueue(SavedDesc); } - else if (Owner->PopAlternativeURI(NewURI)) - { - Owner->FailMessage(Message); - auto &desc = Owner->GetItemDesc(); - if (Log != nullptr) - Log->Fail(desc); - ChangeSiteIsMirrorChange(NewURI, desc, Owner); - desc.URI = NewURI; - if (isDoomedItem(Owner) == false) - OwnerQ->Owner->Enqueue(desc); - } else { - if (errAuthErr && Owner->GetExpectedHashes().empty() == false) - Owner->Status = pkgAcquire::Item::StatAuthError; - else if (errTransient) - Owner->Status = pkgAcquire::Item::StatTransientNetworkError; - auto SavedDesc = Owner->GetItemDesc(); - if (isDoomedItem(Owner) == false) - Owner->Failed(Message, Config); - if (Log != nullptr) - Log->Fail(SavedDesc); + if (errAuthErr) + Owner->RemoveAlternativeSite(URI::SiteOnly(Owner->GetItemDesc().URI)); + if (Owner->PopAlternativeURI(NewURI)) + { + Owner->FailMessage(Message); + auto &desc = Owner->GetItemDesc(); + if (Log != nullptr) + Log->Fail(desc); + ChangeSiteIsMirrorChange(NewURI, desc, Owner); + desc.URI = NewURI; + if (isDoomedItem(Owner) == false) + OwnerQ->Owner->Enqueue(desc); + } + else + { + if (errAuthErr && Owner->GetExpectedHashes().empty() == false) + Owner->Status = pkgAcquire::Item::StatAuthError; + else if (errTransient) + Owner->Status = pkgAcquire::Item::StatTransientNetworkError; + auto SavedDesc = Owner->GetItemDesc(); + if (isDoomedItem(Owner) == false) + Owner->Failed(Message, Config); + if (Log != nullptr) + Log->Fail(SavedDesc); + } } } } diff --git a/test/integration/test-partial-file-support b/test/integration/test-partial-file-support index 9b5eed1e5..88fa91324 100755 --- a/test/integration/test-partial-file-support +++ b/test/integration/test-partial-file-support @@ -24,7 +24,7 @@ testdownloadfile() { else msgpass fi - sed -e '/^ <- / s#%20# #g' -e '/^ <- / s#%0a#\n#g' "$DOWNLOADLOG" | grep '^.*-Hash: ' > receivedhashes.log + sed -e '/^ <- / s#%20# #g' -e '/^ <- / s#%0a#\n#g' "$DOWNLOADLOG" | grep '^.*-Hash: ' > receivedhashes.log || true testsuccess test -s receivedhashes.log local HASHES_OK=0 local HASHES_BAD=0 diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage index 5a650ad83..eec384573 100755 --- a/test/integration/test-pdiff-usage +++ b/test/integration/test-pdiff-usage @@ -398,7 +398,8 @@ testcase -o Acquire::IndexTargets::deb::Packages::KeepCompressed=true partialleftovers() { generatepartialleftovers "redirectme_Packages.${LOWCOSTEXT}" "redirectme_Packages-patched.${LOWCOSTEXT}"; } -webserverconfig 'aptwebserver::redirect::replace::/redirectme/' "http://0.0.0.0:${APTHTTPPORT}/" +# redirect the InRelease file only – the other files are auto-redirected by apt +webserverconfig 'aptwebserver::redirect::replace::/redirectme/I' "http://0.0.0.0:${APTHTTPPORT}/I" rewritesourceslist "http://localhost:${APTHTTPPORT}/redirectme" aptautotest_apt_update() { aptautotest_aptget_update "$@" -- cgit v1.2.3 From ed0e9eadeb3003f4f150da3c463b28cfa5e54b6f Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 15 Jan 2018 12:04:45 +0100 Subject: Handle by-hash URI construction more centrally Individual items shouldn't concern themselves with these alternative locations, we can deal with this more efficiently within the infrastructure created for other alternative URIs now avoiding the need to implement this in each item. --- apt-pkg/acquire-item.cc | 164 ++++++++++++++++---------------------- apt-pkg/acquire-item.h | 25 ++---- test/integration/test-pdiff-usage | 2 + 3 files changed, 76 insertions(+), 115 deletions(-) diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index d0b92ec66..f52cd5b6c 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -398,26 +398,45 @@ bool pkgAcqTransactionItem::QueueURI(pkgAcquire::ItemDesc &Item) Status = StatDone; return false; } + // this ensures we rewrite only once and only the first step + auto const OldBaseURI = Target.Option(IndexTarget::BASE_URI); + if (OldBaseURI.empty() || APT::String::Startswith(Item.URI, OldBaseURI) == false) + return pkgAcquire::Item::QueueURI(Item); + + std::vector urls = {Item.URI}; // If we got the InRelease file via a mirror, pick all indexes directly from this mirror, too if (TransactionManager->BaseURI.empty() == false && TransactionManager->UsedMirror.empty() == false && URI::SiteOnly(Item.URI) != URI::SiteOnly(TransactionManager->BaseURI)) { - // this ensures we rewrite only once and only the first step - auto const OldBaseURI = Target.Option(IndexTarget::BASE_URI); - if (OldBaseURI.empty() == false && APT::String::Startswith(Item.URI, OldBaseURI)) + auto ExtraPath = Item.URI.substr(OldBaseURI.length()); + auto newURI = flCombine(TransactionManager->BaseURI, std::move(ExtraPath)); + if (IsGoodAlternativeURI(newURI)) { - auto const ExtraPath = Item.URI.substr(OldBaseURI.length()); - auto newURI = flCombine(TransactionManager->BaseURI, ExtraPath); - if (IsGoodAlternativeURI(newURI)) - { - PushAlternativeURI(std::string(Item.URI), {}, false); - Item.URI = std::move(newURI); - UsedMirror = TransactionManager->UsedMirror; - if (Item.Description.find(" ") != string::npos) - Item.Description.replace(0, Item.Description.find(" "), UsedMirror); - } + urls.push_back(std::move(newURI)); + UsedMirror = TransactionManager->UsedMirror; + if (Item.Description.find(" ") != string::npos) + Item.Description.replace(0, Item.Description.find(" "), UsedMirror); } } + // add URI and by-hash based on it + auto const Expected = GetExpectedHashes(); + auto const TargetHash = Expected.find(nullptr); + auto const useByHash = AcquireByHash(); + for (auto &&U : urls) + { + PushAlternativeURI(std::string(U), {}, false); + if (useByHash == false || unlikely(TargetHash == nullptr)) + continue; + auto const trailing_slash = U.find_last_of("/"); + if (unlikely(trailing_slash == std::string::npos)) + continue; + auto byhashSuffix = "/by-hash/" + TargetHash->HashType() + "/" + TargetHash->HashValue(); + U.replace(trailing_slash, U.length() - trailing_slash, std::move(byhashSuffix)); + PushAlternativeURI(std::move(U), {}, false); + } + // the last URI added is the first one tried + if (unlikely(PopAlternativeURI(Item.URI) == false)) + return false; return pkgAcquire::Item::QueueURI(Item); } /* The transition manager InRelease itself (or its older sisters-in-law @@ -624,6 +643,26 @@ bool pkgAcqDiffIndex::TransactionState(TransactionStates const state) return true; } /*}}}*/ +// pkgAcqTransactionItem::AcquireByHash and specialisations for child classes /*{{{*/ +bool pkgAcqTransactionItem::AcquireByHash() const +{ + if (TransactionManager->MetaIndexParser == nullptr) + return false; + auto const useByHashConf = Target.Option(IndexTarget::BY_HASH); + if (useByHashConf == "force") + return true; + return StringToBool(useByHashConf) == true && TransactionManager->MetaIndexParser->GetSupportsAcquireByHash(); +} +// pdiff patches have a unique name already, no need for by-hash +bool pkgAcqIndexMergeDiffs::AcquireByHash() const +{ + return false; +} +bool pkgAcqIndexDiffs::AcquireByHash() const +{ + return false; +} + /*}}}*/ class APT_HIDDEN NoActionItem : public pkgAcquire::Item /*{{{*/ /* The sole purpose of this class is having an item which does nothing to @@ -1453,7 +1492,6 @@ void pkgAcqMetaClearSig::QueueIndexes(bool const verify) /*{{{*/ std::set targetsSeen; bool const hasReleaseFile = TransactionManager->MetaIndexParser != NULL; - bool const metaBaseSupportsByHash = hasReleaseFile && TransactionManager->MetaIndexParser->GetSupportsAcquireByHash(); bool hasHashes = true; auto IndexTargets = TransactionManager->MetaIndexParser->GetIndexTargets(); if (hasReleaseFile && verify == false) @@ -1589,15 +1627,6 @@ void pkgAcqMetaClearSig::QueueIndexes(bool const verify) /*{{{*/ if (types.empty() == false) { std::ostringstream os; - // add the special compressiontype byhash first if supported - std::string const useByHashConf = Target.Option(IndexTarget::BY_HASH); - bool useByHash = false; - if(useByHashConf == "force") - useByHash = true; - else - useByHash = StringToBool(useByHashConf) == true && metaBaseSupportsByHash; - if (useByHash == true) - os << "by-hash "; std::copy(types.begin(), types.end()-1, std::ostream_iterator(os, " ")); os << *types.rbegin(); Target.Options["COMPRESSIONTYPES"] = os.str(); @@ -2243,8 +2272,6 @@ pkgAcqDiffIndex::pkgAcqDiffIndex(pkgAcquire * const Owner, CompressionExtensions = os.str(); } } - if (Target.Option(IndexTarget::COMPRESSIONTYPES).find("by-hash") != std::string::npos) - CompressionExtensions = "by-hash " + CompressionExtensions; Init(GetDiffIndexURI(Target), GetDiffIndexFileName(Target.Description), Target.ShortDesc); if(Debug) @@ -2255,7 +2282,7 @@ void pkgAcqDiffIndex::QueueOnIMSHit() const /*{{{*/ { // list cleanup needs to know that this file as well as the already // present index is ours, so we create an empty diff to save it for us - new pkgAcqIndexDiffs(Owner, TransactionManager, Target, Target.URI); + new pkgAcqIndexDiffs(Owner, TransactionManager, Target); } /*}}}*/ static bool RemoveFileForBootstrapLinking(std::string &ErrorText, std::string const &For, std::string const &Boot)/*{{{*/ @@ -2655,31 +2682,14 @@ void pkgAcqDiffIndex::Done(string const &Message,HashStringList const &Hashes, / } else { - // we have something, queue the diffs - string::size_type const last_space = Description.rfind(" "); - if(last_space != string::npos) - Description.erase(last_space, Description.size()-last_space); - - std::string indexURI = Desc.URI; - auto const byhashidx = indexURI.find("/by-hash/"); - if (byhashidx != std::string::npos) - indexURI = indexURI.substr(0, byhashidx - strlen(".diff")); - else - { - auto end = indexURI.length() - strlen(".diff/Index"); - if (CurrentCompressionExtension != "uncompressed") - end -= (1 + CurrentCompressionExtension.length()); - indexURI = indexURI.substr(0, end); - } - if (pdiff_merge == false) - new pkgAcqIndexDiffs(Owner, TransactionManager, Target, indexURI, available_patches); + new pkgAcqIndexDiffs(Owner, TransactionManager, Target, available_patches); else { diffs = new std::vector(available_patches.size()); for(size_t i = 0; i < available_patches.size(); ++i) (*diffs)[i] = new pkgAcqIndexMergeDiffs(Owner, TransactionManager, - Target, indexURI, + Target, available_patches[i], diffs); } @@ -2708,9 +2718,8 @@ pkgAcqDiffIndex::~pkgAcqDiffIndex() pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire *const Owner, pkgAcqMetaClearSig *const TransactionManager, IndexTarget const &Target, - std::string const &indexURI, vector const &diffs) - : pkgAcqBaseIndex(Owner, TransactionManager, Target), indexURI(indexURI), + : pkgAcqBaseIndex(Owner, TransactionManager, Target), available_patches(diffs) { DestFile = GetKeepCompressedFileName(GetPartialFileNameFromURI(Target.URI), Target); @@ -2718,7 +2727,6 @@ pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire *const Owner, Debug = _config->FindB("Debug::pkgAcquire::Diffs",false); Desc.Owner = this; - Description = Target.Description; Desc.ShortDesc = Target.ShortDesc; if(available_patches.empty() == true) @@ -2835,8 +2843,8 @@ bool pkgAcqIndexDiffs::QueueNextDiff() /*{{{*/ } // queue the right diff - Desc.URI = indexURI + ".diff/" + available_patches[0].file + ".gz"; - Desc.Description = Description + " " + available_patches[0].file + string(".pdiff"); + Desc.URI = Target.URI + ".diff/" + available_patches[0].file + ".gz"; + Desc.Description = Target.Description + " " + available_patches[0].file + string(".pdiff"); DestFile = GetKeepCompressedFileName(GetPartialFileNameFromURI(Target.URI + ".diff/" + available_patches[0].file), Target); if(Debug) @@ -2888,7 +2896,7 @@ void pkgAcqIndexDiffs::Done(string const &Message, HashStringList const &Hashes, // see if there is more to download if(available_patches.empty() == false) { - new pkgAcqIndexDiffs(Owner, TransactionManager, Target, indexURI, available_patches); + new pkgAcqIndexDiffs(Owner, TransactionManager, Target, available_patches); Finish(); } else { DestFile = PatchedFile; @@ -2917,19 +2925,17 @@ pkgAcqIndexDiffs::~pkgAcqIndexDiffs() {} pkgAcqIndexMergeDiffs::pkgAcqIndexMergeDiffs(pkgAcquire *const Owner, pkgAcqMetaClearSig *const TransactionManager, IndexTarget const &Target, - std::string const &indexURI, DiffInfo const &patch, std::vector const *const allPatches) - : pkgAcqBaseIndex(Owner, TransactionManager, Target), indexURI(indexURI), + : pkgAcqBaseIndex(Owner, TransactionManager, Target), patch(patch), allPatches(allPatches), State(StateFetchDiff) { Debug = _config->FindB("Debug::pkgAcquire::Diffs",false); - Description = Target.Description; Desc.Owner = this; Desc.ShortDesc = Target.ShortDesc; - Desc.URI = indexURI + ".diff/" + patch.file + ".gz"; - Desc.Description = Description + " " + patch.file + ".pdiff"; + Desc.URI = Target.URI + ".diff/" + patch.file + ".gz"; + Desc.Description = Target.Description + " " + patch.file + ".pdiff"; DestFile = GetPartialFileNameFromURI(Target.URI + ".diff/" + patch.file + ".gz"); if(Debug) @@ -3085,60 +3091,28 @@ pkgAcqIndex::pkgAcqIndex(pkgAcquire * const Owner, } /*}}}*/ // AcqIndex::Init - deferred Constructor /*{{{*/ -static void NextCompressionExtension(std::string &CurrentCompressionExtension, std::string &CompressionExtensions, bool const preview) +void pkgAcqIndex::Init(string const &URI, string const &URIDesc, + string const &ShortDesc) { + Stage = STAGE_DOWNLOAD; + + DestFile = GetPartialFileNameFromURI(URI); size_t const nextExt = CompressionExtensions.find(' '); if (nextExt == std::string::npos) { CurrentCompressionExtension = CompressionExtensions; - if (preview == false) - CompressionExtensions.clear(); + CompressionExtensions.clear(); } else { CurrentCompressionExtension = CompressionExtensions.substr(0, nextExt); - if (preview == false) - CompressionExtensions = CompressionExtensions.substr(nextExt+1); + CompressionExtensions = CompressionExtensions.substr(nextExt+1); } -} -void pkgAcqIndex::Init(string const &URI, string const &URIDesc, - string const &ShortDesc) -{ - Stage = STAGE_DOWNLOAD; - - DestFile = GetPartialFileNameFromURI(URI); - NextCompressionExtension(CurrentCompressionExtension, CompressionExtensions, false); if (CurrentCompressionExtension == "uncompressed") { Desc.URI = URI; } - else if (CurrentCompressionExtension == "by-hash") - { - NextCompressionExtension(CurrentCompressionExtension, CompressionExtensions, true); - if(unlikely(CurrentCompressionExtension.empty())) - return; - if (CurrentCompressionExtension != "uncompressed") - { - Desc.URI = URI + '.' + CurrentCompressionExtension; - DestFile = DestFile + '.' + CurrentCompressionExtension; - } - else - Desc.URI = URI; - - HashStringList const Hashes = GetExpectedHashes(); - HashString const * const TargetHash = Hashes.find(NULL); - if (unlikely(TargetHash == nullptr)) - return; - std::string const ByHash = "/by-hash/" + TargetHash->HashType() + "/" + TargetHash->HashValue(); - size_t const trailing_slash = Desc.URI.find_last_of("/"); - if (unlikely(trailing_slash == std::string::npos)) - return; - Desc.URI = Desc.URI.replace( - trailing_slash, - Desc.URI.substr(trailing_slash+1).size()+1, - ByHash); - } else if (unlikely(CurrentCompressionExtension.empty())) return; else diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index 814596850..3a5a518c2 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -407,7 +407,7 @@ class APT_HIDDEN pkgAcqTransactionItem: public pkgAcquire::Item /*{{{*/ virtual HashStringList GetExpectedHashes() const APT_OVERRIDE; virtual std::string GetMetaKey() const; virtual bool HashesRequired() const APT_OVERRIDE; - + virtual bool AcquireByHash() const; pkgAcqTransactionItem(pkgAcquire * const Owner, pkgAcqMetaClearSig * const TransactionManager, IndexTarget const &Target) APT_NONNULL(2, 3); virtual ~pkgAcqTransactionItem(); @@ -730,11 +730,6 @@ class APT_HIDDEN pkgAcqDiffIndex : public pkgAcqIndex /** \brief If \b true, debugging information will be written to std::clog. */ bool Debug; - /** \brief A description of the Packages file (stored in - * pkgAcquire::ItemDesc::Description). - */ - std::string Description; - /** \brief Get the full pathname of the final file for the current URI */ virtual std::string GetFinalFilename() const APT_OVERRIDE; @@ -792,8 +787,6 @@ class APT_HIDDEN pkgAcqDiffIndex : public pkgAcqIndex */ class APT_HIDDEN pkgAcqIndexMergeDiffs : public pkgAcqBaseIndex { - std::string const indexURI; - protected: /** \brief If \b true, debugging output will be written to @@ -801,9 +794,6 @@ class APT_HIDDEN pkgAcqIndexMergeDiffs : public pkgAcqBaseIndex */ bool Debug; - /** \brief description of the file being downloaded. */ - std::string Description; - /** \brief information about the current patch */ struct DiffInfo const patch; @@ -839,6 +829,7 @@ class APT_HIDDEN pkgAcqIndexMergeDiffs : public pkgAcqBaseIndex virtual std::string DescURI() const APT_OVERRIDE {return Target.URI + "Index";}; virtual HashStringList GetExpectedHashes() const APT_OVERRIDE; virtual bool HashesRequired() const APT_OVERRIDE; + virtual bool AcquireByHash() const APT_OVERRIDE; /** \brief Create an index merge-diff item. * @@ -853,9 +844,8 @@ class APT_HIDDEN pkgAcqIndexMergeDiffs : public pkgAcqBaseIndex * check if it was the last one to complete the download step */ pkgAcqIndexMergeDiffs(pkgAcquire *const Owner, pkgAcqMetaClearSig *const TransactionManager, - IndexTarget const &Target, - std::string const &indexURI, DiffInfo const &patch, - std::vector const *const allPatches) APT_NONNULL(2, 3, 7); + IndexTarget const &Target, DiffInfo const &patch, + std::vector const *const allPatches) APT_NONNULL(2, 3, 6); virtual ~pkgAcqIndexMergeDiffs(); }; /*}}}*/ @@ -872,8 +862,6 @@ class APT_HIDDEN pkgAcqIndexMergeDiffs : public pkgAcqBaseIndex */ class APT_HIDDEN pkgAcqIndexDiffs : public pkgAcqBaseIndex { - std::string const indexURI; - private: /** \brief Queue up the next diff download. @@ -907,9 +895,6 @@ class APT_HIDDEN pkgAcqIndexDiffs : public pkgAcqBaseIndex */ bool Debug; - /** A description of the file being downloaded. */ - std::string Description; - /** The patches that remain to be downloaded, including the patch * being downloaded right now. This list should be ordered so * that each diff appears before any diff that depends on it. @@ -945,6 +930,7 @@ class APT_HIDDEN pkgAcqIndexDiffs : public pkgAcqBaseIndex virtual std::string DescURI() const APT_OVERRIDE {return Target.URI + "IndexDiffs";}; virtual HashStringList GetExpectedHashes() const APT_OVERRIDE; virtual bool HashesRequired() const APT_OVERRIDE; + virtual bool AcquireByHash() const APT_OVERRIDE; /** \brief Create an index diff item. * @@ -961,7 +947,6 @@ class APT_HIDDEN pkgAcqIndexDiffs : public pkgAcqBaseIndex */ pkgAcqIndexDiffs(pkgAcquire *const Owner, pkgAcqMetaClearSig *const TransactionManager, IndexTarget const &Target, - std::string const &indexURI, std::vector const &diffs = std::vector()) APT_NONNULL(2, 3); virtual ~pkgAcqIndexDiffs(); }; diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage index eec384573..7cda2ee45 100755 --- a/test/integration/test-pdiff-usage +++ b/test/integration/test-pdiff-usage @@ -138,6 +138,8 @@ SHA256-Download: mkdir -p "${BYHASH}" find "${NORMAL}/" -maxdepth 1 -name "Index*" -exec mv '{}' "$BYHASH" \; ln -s "${BYHASH}/Index.gz" "${BYHASH}/$(sha256sum "${BYHASH}/Index.gz" | cut -f1 -d' ')" + echo 'foobar' > "${BYHASH}/$(sha256sum "$PATCHFILE" | cut -f1 -d' ')" + echo 'foobar' > "${BYHASH}/$(sha256sum "${PATCHFILE}.gz" | cut -f1 -d' ')" rm -rf rootdir/var/lib/apt/lists cp -a rootdir/var/lib/apt/lists-bak rootdir/var/lib/apt/lists wasmergeused "$@" -o Acquire::By-Hash=force -- cgit v1.2.3 From 97f0bb4a60f2a3eda093e015767211d5c3c21c32 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 25 Jan 2018 20:52:31 +0100 Subject: Don't force the same mirror for by-hash URIs Downloading from the same mirror we got a Release file from makes sense for non-unique URIs as their content changes between mirror states, but if we ask for an index via by-hash we can be sure that we either get the file we wanted or a 404 for which we can perform a fallback for which allows us to pull indexes from different mirror in parallel. --- apt-pkg/acquire-item.cc | 59 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index f52cd5b6c..a366b8981 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -402,9 +402,10 @@ bool pkgAcqTransactionItem::QueueURI(pkgAcquire::ItemDesc &Item) auto const OldBaseURI = Target.Option(IndexTarget::BASE_URI); if (OldBaseURI.empty() || APT::String::Startswith(Item.URI, OldBaseURI) == false) return pkgAcquire::Item::QueueURI(Item); - - std::vector urls = {Item.URI}; + // the given URI is our last resort + PushAlternativeURI(std::string(Item.URI), {}, false); // If we got the InRelease file via a mirror, pick all indexes directly from this mirror, too + std::string SameMirrorURI; if (TransactionManager->BaseURI.empty() == false && TransactionManager->UsedMirror.empty() == false && URI::SiteOnly(Item.URI) != URI::SiteOnly(TransactionManager->BaseURI)) { @@ -412,31 +413,51 @@ bool pkgAcqTransactionItem::QueueURI(pkgAcquire::ItemDesc &Item) auto newURI = flCombine(TransactionManager->BaseURI, std::move(ExtraPath)); if (IsGoodAlternativeURI(newURI)) { - urls.push_back(std::move(newURI)); - UsedMirror = TransactionManager->UsedMirror; - if (Item.Description.find(" ") != string::npos) - Item.Description.replace(0, Item.Description.find(" "), UsedMirror); + SameMirrorURI = std::move(newURI); + PushAlternativeURI(std::string(SameMirrorURI), {}, false); } } // add URI and by-hash based on it - auto const Expected = GetExpectedHashes(); - auto const TargetHash = Expected.find(nullptr); - auto const useByHash = AcquireByHash(); - for (auto &&U : urls) + if (AcquireByHash()) { - PushAlternativeURI(std::string(U), {}, false); - if (useByHash == false || unlikely(TargetHash == nullptr)) - continue; - auto const trailing_slash = U.find_last_of("/"); - if (unlikely(trailing_slash == std::string::npos)) - continue; - auto byhashSuffix = "/by-hash/" + TargetHash->HashType() + "/" + TargetHash->HashValue(); - U.replace(trailing_slash, U.length() - trailing_slash, std::move(byhashSuffix)); - PushAlternativeURI(std::move(U), {}, false); + // if we use the mirror transport, ask it for by-hash uris + // we need to stick to the same mirror only for non-unique filenames + auto const sameMirrorException = [&]() { + if (Item.URI.find("mirror") == std::string::npos) + return false; + ::URI uri(Item.URI); + return uri.Access == "mirror" || APT::String::Startswith(uri.Access, "mirror+") || + APT::String::Endswith(uri.Access, "+mirror") || uri.Access.find("+mirror+") != std::string::npos; + }(); + if (sameMirrorException) + SameMirrorURI.clear(); + // now add the actual by-hash uris + auto const Expected = GetExpectedHashes(); + auto const TargetHash = Expected.find(nullptr); + auto const PushByHashURI = [&](std::string U) { + if (unlikely(TargetHash == nullptr)) + return false; + auto const trailing_slash = U.find_last_of("/"); + if (unlikely(trailing_slash == std::string::npos)) + return false; + auto byhashSuffix = "/by-hash/" + TargetHash->HashType() + "/" + TargetHash->HashValue(); + U.replace(trailing_slash, U.length() - trailing_slash, std::move(byhashSuffix)); + PushAlternativeURI(std::move(U), {}, false); + return true; + }; + PushByHashURI(Item.URI); + if (SameMirrorURI.empty() == false && PushByHashURI(SameMirrorURI) == false) + SameMirrorURI.clear(); } // the last URI added is the first one tried if (unlikely(PopAlternativeURI(Item.URI) == false)) return false; + if (SameMirrorURI.empty() == false) + { + UsedMirror = TransactionManager->UsedMirror; + if (Item.Description.find(" ") != string::npos) + Item.Description.replace(0, Item.Description.find(" "), UsedMirror); + } return pkgAcquire::Item::QueueURI(Item); } /* The transition manager InRelease itself (or its older sisters-in-law -- cgit v1.2.3