From 07cd99066c30e70a9f41851c80e7c51f4e507163 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 17 Jul 2017 10:52:09 +0200 Subject: support multiline values in LookupTag LookupTag is a little helper to deal with rfc822-style strings we use in apt e.g. to pass acquire messages around for cases in which our usual rfc822 parser is too heavy. All the fields it had to deal with so far were single line, but if they aren't it should really produce the right output and not just return the first line. Error messages are a prime candidate for becoming multiline as at the moment they are stripped of potential newlines due to the previous insufficiency of LookupTag. --- test/libapt/strutil_test.cc | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) (limited to 'test') diff --git a/test/libapt/strutil_test.cc b/test/libapt/strutil_test.cc index 9c192a58b..f531c2bf9 100644 --- a/test/libapt/strutil_test.cc +++ b/test/libapt/strutil_test.cc @@ -319,3 +319,45 @@ TEST(StrUtilTest,RFC1123StrToTime) EXPECT_FALSE(RFC1123StrToTime("Sunday, 06-Nov-94 08:49:37 -0100", t)); EXPECT_FALSE(RFC1123StrToTime("Sunday, 06-Nov-94 08:49:37 -0.1", t)); } +TEST(StrUtilTest, LookupTag) +{ + EXPECT_EQ("", LookupTag("", "Field", "")); + EXPECT_EQ("", LookupTag("", "Field", nullptr)); + EXPECT_EQ("default", LookupTag("", "Field", "default")); + EXPECT_EQ("default", LookupTag("Field1: yes", "Field", "default")); + EXPECT_EQ("default", LookupTag("Fiel: yes", "Field", "default")); + EXPECT_EQ("default", LookupTag("Fiel d: yes", "Field", "default")); + EXPECT_EQ("foo", LookupTag("Field: foo", "Field", "default")); + EXPECT_EQ("foo", LookupTag("Field: foo\n", "Field", "default")); + EXPECT_EQ("foo", LookupTag("\nField: foo\n", "Field", "default")); + EXPECT_EQ("foo", LookupTag("Field:foo", "Field", "default")); + EXPECT_EQ("foo", LookupTag("Field:foo\n", "Field", "default")); + EXPECT_EQ("foo", LookupTag("\nField:foo\n", "Field", "default")); + EXPECT_EQ("foo", LookupTag("Field:\tfoo\n", "Field", "default")); + EXPECT_EQ("foo", LookupTag("Field: foo \t", "Field", "default")); + EXPECT_EQ("foo", LookupTag("Field: foo \t\n", "Field", "default")); + EXPECT_EQ("Field : yes", LookupTag("Field: Field : yes \t\n", "Field", "default")); + EXPECT_EQ("Field : yes", LookupTag("Field:\n Field : yes \t\n", "Field", "default")); + EXPECT_EQ("Field : yes", LookupTag("Foo: bar\nField: Field : yes \t\n", "Field", "default")); + EXPECT_EQ("line1\nline2", LookupTag("Multi: line1\n line2", "Multi", "default")); + EXPECT_EQ("line1\nline2", LookupTag("Multi: line1\n line2\n", "Multi", "default")); + EXPECT_EQ("line1\nline2", LookupTag("Multi:\n line1\n line2\n", "Multi", "default")); + EXPECT_EQ("line1\n\nline2", LookupTag("Multi:\n line1\n .\n line2\n", "Multi", "default")); + EXPECT_EQ("line1\na\nline2", LookupTag("Multi:\n line1\n a\n line2\n", "Multi", "default")); + EXPECT_EQ("line1\nfoo\nline2", LookupTag("Multi:\n line1\n foo\n line2\n", "Multi", "default")); + EXPECT_EQ("line1\n line2", LookupTag("Multi: line1\n line2", "Multi", "default")); + EXPECT_EQ(" line1\n \t line2", LookupTag("Multi:\t \n line1\n \t line2\n", "Multi", "default")); + EXPECT_EQ(" line1\n\n\n \t line2", LookupTag("Multi:\t \n line1\n .\n . \n \t line2\n", "Multi", "default")); + + std::string const msg = + "Field1: Value1\nField2:Value2\nField3:\t Value3\n" + "Multi-Field1: Line1\n Line2\nMulti-Field2:\n Line1\n Line2\n" + "Field4: Value4\nField5:Value5"; + EXPECT_EQ("Value1", LookupTag(msg, "Field1", "")); + EXPECT_EQ("Value2", LookupTag(msg, "Field2", "")); + EXPECT_EQ("Value3", LookupTag(msg, "Field3", "")); + EXPECT_EQ("Line1\nLine2", LookupTag(msg, "Multi-Field1", "")); + EXPECT_EQ("Line1\nLine2", LookupTag(msg, "Multi-Field2", "")); + EXPECT_EQ("Value4", LookupTag(msg, "Field4", "")); + EXPECT_EQ("Value5", LookupTag(msg, "Field5", "")); +} -- cgit v1.2.3-70-g09d2 From dff555d40bb9776b5b809e06527e46b15e78736c Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 26 Oct 2017 01:09:48 +0200 Subject: implement Acquire::Retries support for all items Moving the Retry-implementation from individual items to the worker implementation not only gives every file retry capability instead of just a selected few but also avoids needing to implement it in each item (incorrectly). --- apt-pkg/acquire-item.cc | 66 ++++++++++-------------- apt-pkg/acquire-item.h | 5 ++ apt-pkg/acquire-worker.cc | 34 ++++++++---- test/integration/framework | 8 +-- test/integration/test-bug-869859-retry-downloads | 37 +++++++++++++ test/interactive-helper/aptwebserver.cc | 17 +++++- 6 files changed, 113 insertions(+), 54 deletions(-) create mode 100755 test/integration/test-bug-869859-retry-downloads (limited to 'test') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index b3eb75d16..8c11337ac 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -684,6 +684,11 @@ class pkgAcquire::Item::Private { public: std::vector PastRedirections; + unsigned int Retries; + + Private() : Retries(_config->FindI("Acquire::Retries", 0)) + { + } }; APT_IGNORE_DEPRECATED_PUSH pkgAcquire::Item::Item(pkgAcquire * const owner) : @@ -707,6 +712,11 @@ std::string pkgAcquire::Item::Custom600Headers() const /*{{{*/ return std::string(); } /*}}}*/ +unsigned int &pkgAcquire::Item::ModifyRetries() /*{{{*/ +{ + return d->Retries; +} + /*}}}*/ std::string pkgAcquire::Item::ShortDesc() const /*{{{*/ { return DescURI(); @@ -778,6 +788,13 @@ void pkgAcquire::Item::Failed(string const &Message,pkgAcquire::MethodConfig con Dequeue(); } + FailMessage(Message); + + if (QueueCounter > 1) + Status = StatIdle; +} +void pkgAcquire::Item::FailMessage(string const &Message) +{ string const FailReason = LookupTag(Message, "FailReason"); enum { MAXIMUM_SIZE_EXCEEDED, HASHSUM_MISMATCH, WEAK_HASHSUMS, REDIRECTION_LOOP, OTHER } failreason = OTHER; if ( FailReason == "MaximumSizeExceeded") @@ -851,9 +868,6 @@ void pkgAcquire::Item::Failed(string const &Message,pkgAcquire::MethodConfig con ReportMirrorFailureToCentral(*this, FailReason, ErrorText); else ReportMirrorFailureToCentral(*this, ErrorText, ErrorText); - - if (QueueCounter > 1) - Status = StatIdle; } /*}}}*/ // Acquire::Item::Start - Item has begun to download /*{{{*/ @@ -899,7 +913,7 @@ void pkgAcquire::Item::Done(string const &/*Message*/, HashStringList const &Has } } Status = StatDone; - ErrorText = string(); + ErrorText.clear(); Owner->Dequeue(this); } /*}}}*/ @@ -3217,8 +3231,6 @@ pkgAcqArchive::pkgAcqArchive(pkgAcquire * const Owner,pkgSourceList * const Sour StoreFilename(StoreFilename), Vf(Version.FileList()), Trusted(false) { - Retries = _config->FindI("Acquire::Retries",0); - if (Version.Arch() == 0) { _error->Error(_("I wasn't able to locate a file for the %s package. " @@ -3453,17 +3465,6 @@ void pkgAcqArchive::Failed(string const &Message,pkgAcquire::MethodConfig const Status = StatIdle; if (QueueNext() == false) { - // This is the retry counter - if (Retries != 0 && - Cnf->LocalOnly == false && - StringToBool(LookupTag(Message,"Transient-Failure"),false) == true) - { - Retries--; - Vf = Version.FileList(); - if (QueueNext() == true) - return; - } - StoreFilename = string(); Status = StatError; } @@ -3754,14 +3755,12 @@ pkgAcqChangelog::~pkgAcqChangelog() /*{{{*/ /*}}}*/ // AcqFile::pkgAcqFile - Constructor /*{{{*/ -pkgAcqFile::pkgAcqFile(pkgAcquire * const Owner,string const &URI, HashStringList const &Hashes, - unsigned long long const Size,string const &Dsc,string const &ShortDesc, +APT_IGNORE_DEPRECATED_PUSH +pkgAcqFile::pkgAcqFile(pkgAcquire *const Owner, string const &URI, HashStringList const &Hashes, + unsigned long long const Size, string const &Dsc, string const &ShortDesc, const string &DestDir, const string &DestFilename, - bool const IsIndexFile) : - Item(Owner), d(NULL), IsIndexFile(IsIndexFile), ExpectedHashes(Hashes) + bool const IsIndexFile) : Item(Owner), d(NULL), Retries(0), IsIndexFile(IsIndexFile), ExpectedHashes(Hashes) { - Retries = _config->FindI("Acquire::Retries",0); - if(!DestFilename.empty()) DestFile = DestFilename; else if(!DestDir.empty()) @@ -3791,6 +3790,7 @@ pkgAcqFile::pkgAcqFile(pkgAcquire * const Owner,string const &URI, HashStringLis QueueURI(Desc); } +APT_IGNORE_DEPRECATED_POP /*}}}*/ // AcqFile::Done - Item downloaded OK /*{{{*/ void pkgAcqFile::Done(string const &Message,HashStringList const &CalcHashes, @@ -3840,24 +3840,10 @@ void pkgAcqFile::Done(string const &Message,HashStringList const &CalcHashes, } } /*}}}*/ -// AcqFile::Failed - Failure handler /*{{{*/ -// --------------------------------------------------------------------- -/* Here we try other sources */ -void pkgAcqFile::Failed(string const &Message, pkgAcquire::MethodConfig const * const Cnf) +void pkgAcqFile::Failed(string const &Message, pkgAcquire::MethodConfig const *const Cnf) /*{{{*/ { - Item::Failed(Message,Cnf); - - // This is the retry counter - if (Retries != 0 && - Cnf->LocalOnly == false && - StringToBool(LookupTag(Message,"Transient-Failure"),false) == true) - { - --Retries; - QueueURI(Desc); - Status = StatIdle; - return; - } - + // FIXME: Remove this pointless overload on next ABI break + Item::Failed(Message, Cnf); } /*}}}*/ string pkgAcqFile::Custom600Headers() const /*{{{*/ diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index a5c7d848a..7705f3ccb 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -174,6 +174,7 @@ class pkgAcquire::Item : public WeakPointable /*{{{*/ * \sa pkgAcqMethod */ virtual void Failed(std::string const &Message,pkgAcquire::MethodConfig const * const Cnf); + APT_HIDDEN void FailMessage(std::string const &Message); /** \brief Invoked by the acquire worker to check if the successfully * fetched object is also the objected we wanted to have. @@ -238,6 +239,8 @@ class pkgAcquire::Item : public WeakPointable /*{{{*/ * no trailing newline. */ virtual std::string Custom600Headers() const; + // Retries should really be a member of the Item, but can't be for ABI reasons + APT_HIDDEN unsigned int &ModifyRetries(); /** \brief A "descriptive" URI-like string. * @@ -994,6 +997,7 @@ class pkgAcqArchive : public pkgAcquire::Item * * Set from Acquire::Retries. */ + APT_DEPRECATED_MSG("Unused member. See pkgAcqItem::Retries.") unsigned int Retries; /** \brief \b true if this version file is being downloaded from a @@ -1171,6 +1175,7 @@ class pkgAcqFile : public pkgAcquire::Item /** \brief How many times to retry the download, set from * Acquire::Retries. */ + APT_DEPRECATED_MSG("Unused member. See pkgAcqItem::Retries.") unsigned int Retries; /** \brief Should this file be considered a index file */ diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc index 39158aed7..a763d9242 100644 --- a/apt-pkg/acquire-worker.cc +++ b/apt-pkg/acquire-worker.cc @@ -506,6 +506,9 @@ bool pkgAcquire::Worker::RunMessages() Itm = nullptr; bool errTransient = false, errAuthErr = false; + if (StringToBool(LookupTag(Message, "Transient-Failure"), false) == true) + errTransient = true; + else { std::string const failReason = LookupTag(Message, "FailReason"); { @@ -522,15 +525,28 @@ bool pkgAcquire::Worker::RunMessages() for (auto const Owner: ItmOwners) { - 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 (errTransient == true && Config->LocalOnly == false && Owner->ModifyRetries() != 0) + { + --Owner->ModifyRetries(); + Owner->FailMessage(Message); + auto SavedDesc = Owner->GetItemDesc(); + if (Log != nullptr) + Log->Fail(SavedDesc); + if (isDoomedItem(Owner) == false) + OwnerQ->Owner->Enqueue(SavedDesc); + } + 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); + } } ItemDone(); diff --git a/test/integration/framework b/test/integration/framework index bf6a8155e..bff6c0e65 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -1273,8 +1273,8 @@ webserverconfig() { NOCHECK=true shift fi - local DOWNLOG='rootdir/tmp/download-testfile.log' - local STATUS='downloaded/webserverconfig.status' + local DOWNLOG="${TMPWORKINGDIRECTORY}/rootdir/tmp/download-testfile.log" + local STATUS="${TMPWORKINGDIRECTORY}/downloaded/webserverconfig.status" rm -f "$STATUS" "$DOWNLOG" # very very basic URI encoding local URI @@ -1937,8 +1937,8 @@ testaccessrights() { testwebserverlaststatuscode() { msggroup 'testwebserverlaststatuscode' - local DOWNLOG='rootdir/tmp/webserverstatus-testfile.log' - local STATUS='downloaded/webserverstatus-statusfile.log' + local DOWNLOG="${TMPWORKINGDIRECTORY}/rootdir/tmp/webserverstatus-testfile.log" + local STATUS="${TMPWORKINGDIRECTORY}/downloaded/webserverstatus-statusfile.log" rm -f "$DOWNLOG" "$STATUS" msgtest 'Test last status code from the webserver was' "$1" if downloadfile "http://localhost:${APTHTTPPORT}/_config/find/aptwebserver::last-status-code" "$STATUS" > "$DOWNLOG" && [ "$(cat "$STATUS")" = "$1" ]; then diff --git a/test/integration/test-bug-869859-retry-downloads b/test/integration/test-bug-869859-retry-downloads new file mode 100755 index 000000000..a62429a53 --- /dev/null +++ b/test/integration/test-bug-869859-retry-downloads @@ -0,0 +1,37 @@ +#!/bin/sh +set -e + +TESTDIR="$(readlink -f "$(dirname "$0")")" +. "$TESTDIR/framework" + +setupenvironment +configarchitecture 'amd64' + +buildsimplenativepackage 'testpkg' 'all' '1' 'stable' + +setupaptarchive --no-update +changetowebserver +testsuccess apt update + +cd downloaded +testsuccess apt download testpkg +testsuccess test -f testpkg_1_all.deb +rm -f testpkg_1_all.deb + +msgmsg 'Fail after too many retries' +webserverconfig 'aptwebserver::failrequest' '429' +webserverconfig 'aptwebserver::failrequest::pool/testpkg_1_all.deb' '99' +testfailure apt download testpkg -o acquire::retries=3 +testfailure test -f testpkg_1_all.deb + +msgmsg 'Success in the third try' +webserverconfig 'aptwebserver::failrequest::pool/testpkg_1_all.deb' '2' +testsuccess apt download testpkg -o acquire::retries=3 +testsuccess test -f testpkg_1_all.deb +rm -f testpkg_1_all.deb + +msgmsg 'Do not try everything again, hard failures keep hard failures' +webserverconfig 'aptwebserver::failrequest' '404' +webserverconfig 'aptwebserver::failrequest::pool/testpkg_1_all.deb' '2' +testfailure apt download testpkg -o acquire::retries=3 +testfailure test -f testpkg_1_all.deb diff --git a/test/interactive-helper/aptwebserver.cc b/test/interactive-helper/aptwebserver.cc index 11f9b4b4f..4bc344178 100644 --- a/test/interactive-helper/aptwebserver.cc +++ b/test/interactive-helper/aptwebserver.cc @@ -82,7 +82,10 @@ static std::string httpcodeToStr(int const httpcode) /*{{{*/ case 504: return _config->Find("aptwebserver::httpcode::504", "504 Gateway Time-out"); case 505: return _config->Find("aptwebserver::httpcode::505", "505 HTTP Version not supported"); } - return ""; + std::string codeconf, code; + strprintf(codeconf, "aptwebserver::httpcode::%i", httpcode); + strprintf(code, "%i Unknown HTTP code", httpcode); + return _config->Find(codeconf, code); } /*}}}*/ static bool chunkedTransferEncoding(std::list const &headers) { @@ -696,6 +699,18 @@ static void * handleClient(int const client, size_t const id) /*{{{*/ } } + // automatic retry can be tested with this + { + int failrequests = _config->FindI("aptwebserver::failrequest::" + filename, 0); + if (failrequests != 0) + { + --failrequests; + _config->Set(("aptwebserver::failrequest::" + filename).c_str(), failrequests); + sendError(log, client, _config->FindI("aptwebserver::failrequest", 400), *m, sendContent, "Server is configured to fail this file.", headers); + continue; + } + } + // deal with the request unsigned int const httpsport = _config->FindI("aptwebserver::port::https", 4433); std::string hosthttpsport; -- cgit v1.2.3-70-g09d2 From 355e1aceac1dd05c4c7daf3420b09bd860fd169d Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 27 Oct 2017 19:09:45 +0200 Subject: implement fallback to alternative URIs for all items For deb files we always supported falling back from one server to the other if one failed to download the deb, but that was hardwired in the handling of this specific item. Moving this alongside the retry infrastructure we can implement it for all items and allow methods to use this as well by providing additional URIs in a redirect. --- apt-pkg/acquire-item.cc | 308 ++++++++++++----------- apt-pkg/acquire-item.h | 4 + apt-pkg/acquire-worker.cc | 53 +++- apt-pkg/indexfile.cc | 4 + apt-pkg/indexfile.h | 1 + test/integration/framework | 2 +- test/integration/test-bug-869859-retry-downloads | 18 ++ 7 files changed, 230 insertions(+), 160 deletions(-) (limited to 'test') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 0e73b3b8c..dc45a6acd 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -683,6 +683,13 @@ class APT_HIDDEN CleanupItem : public pkgAcqTransactionItem /*{{{*/ class pkgAcquire::Item::Private { public: + struct AlternateURI + { + std::string const URI; + std::unordered_map changefields; + AlternateURI(std::string &&u, decltype(changefields) &&cf) : URI(u), changefields(cf) {} + }; + std::list AlternativeURIs; std::vector PastRedirections; std::unordered_map CustomFields; unsigned int Retries; @@ -723,6 +730,32 @@ std::unordered_map &pkgAcquire::Item::ModifyCustomFiel return d->CustomFields; } /*}}}*/ +bool pkgAcquire::Item::PopAlternativeURI(std::string &NewURI) /*{{{*/ +{ + if (d->AlternativeURIs.empty()) + return false; + auto const AltUri = d->AlternativeURIs.front(); + d->AlternativeURIs.pop_front(); + NewURI = AltUri.URI; + auto &CustomFields = ModifyCustomFields(); + for (auto const &f : AltUri.changefields) + { + if (f.second.empty()) + CustomFields.erase(f.first); + else + CustomFields[f.first] = f.second; + } + return true; +} + /*}}}*/ +void pkgAcquire::Item::PushAlternativeURI(std::string &&NewURI, std::unordered_map &&fields, bool const at_the_back) /*{{{*/ +{ + 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)); +} + /*}}}*/ unsigned int &pkgAcquire::Item::ModifyRetries() /*{{{*/ { return d->Retries; @@ -1070,6 +1103,7 @@ pkgAcqTransactionItem::pkgAcqTransactionItem(pkgAcquire * const Owner, /*{{{*/ {"Target-Release", Target.Option(IndexTarget::RELEASE)}, {"Target-Architecture", Target.Option(IndexTarget::ARCHITECTURE)}, {"Target-Language", Target.Option(IndexTarget::LANGUAGE)}, + {"Target-Type", "index"}, }; } /*}}}*/ @@ -3244,12 +3278,12 @@ pkgAcqIndex::~pkgAcqIndex() {} // --------------------------------------------------------------------- /* This just sets up the initial fetch environment and queues the first possibilitiy */ -pkgAcqArchive::pkgAcqArchive(pkgAcquire * const Owner,pkgSourceList * const Sources, - pkgRecords * const Recs,pkgCache::VerIterator const &Version, - string &StoreFilename) : - Item(Owner), d(NULL), LocalSource(false), Version(Version), Sources(Sources), Recs(Recs), - StoreFilename(StoreFilename), Vf(Version.FileList()), - Trusted(false) +APT_IGNORE_DEPRECATED_PUSH +pkgAcqArchive::pkgAcqArchive(pkgAcquire *const Owner, pkgSourceList *const Sources, + pkgRecords *const Recs, pkgCache::VerIterator const &Version, + string &StoreFilename) : Item(Owner), d(NULL), LocalSource(false), Version(Version), Sources(Sources), Recs(Recs), + StoreFilename(StoreFilename), Vf(), + Trusted(false) { if (Version.Arch() == 0) { @@ -3259,32 +3293,6 @@ pkgAcqArchive::pkgAcqArchive(pkgAcquire * const Owner,pkgSourceList * const Sour Version.ParentPkg().FullName().c_str()); return; } - - /* We need to find a filename to determine the extension. We make the - assumption here that all the available sources for this version share - the same extension.. */ - // Skip not source sources, they do not have file fields. - for (; Vf.end() == false; ++Vf) - { - if (Vf.File().Flagged(pkgCache::Flag::NotSource)) - continue; - break; - } - - // Does not really matter here.. we are going to fail out below - if (Vf.end() != true) - { - // If this fails to get a file name we will bomb out below. - pkgRecords::Parser &Parse = Recs->Lookup(Vf); - if (_error->PendingError() == true) - return; - - // Generate the final file name as: package_version_arch.foo - StoreFilename = QuoteString(Version.ParentPkg().Name(),"_:") + '_' + - QuoteString(Version.VerStr(),"_:") + '_' + - QuoteString(Version.Arch(),"_:.") + - "." + flExtension(Parse.FileName()); - } // check if we have one trusted source for the package. if so, switch // to "TrustedOnly" mode - but only if not in AllowUnauthenticated mode @@ -3317,58 +3325,60 @@ pkgAcqArchive::pkgAcqArchive(pkgAcquire * const Owner,pkgSourceList * const Sour if (allowUnauth == true && seenUntrusted == true) Trusted = false; - // Select a source - if (QueueNext() == false && _error->PendingError() == false) - _error->Error(_("Can't find a source to download version '%s' of '%s'"), - Version.VerStr(), Version.ParentPkg().FullName(false).c_str()); -} - /*}}}*/ -// AcqArchive::QueueNext - Queue the next file source /*{{{*/ -// --------------------------------------------------------------------- -/* This queues the next available file version for download. It checks if - the archive is already available in the cache and stashs the MD5 for - checking later. */ -bool pkgAcqArchive::QueueNext() -{ - for (; Vf.end() == false; ++Vf) + StoreFilename.clear(); + std::set targetComponents, targetCodenames, targetSuites; + for (auto Vf = Version.FileList(); Vf.end() == false; ++Vf) { - pkgCache::PkgFileIterator const PkgF = Vf.File(); - // Ignore not source sources + auto const PkgF = Vf.File(); + if (unlikely(PkgF.end())) + continue; if (PkgF.Flagged(pkgCache::Flag::NotSource)) continue; - - // Try to cross match against the source list pkgIndexFile *Index; if (Sources->FindIndex(PkgF, Index) == false) - continue; - LocalSource = PkgF.Flagged(pkgCache::Flag::LocalSource); - - // only try to get a trusted package from another source if that source - // is also trusted - if(Trusted && !Index->IsTrusted()) + continue; + if (Trusted && Index->IsTrusted() == false) continue; - // Grab the text package record pkgRecords::Parser &Parse = Recs->Lookup(Vf); - if (_error->PendingError() == true) - return false; - - string PkgFile = Parse.FileName(); - ExpectedHashes = Parse.Hashes(); - - if (PkgFile.empty() == true) - return _error->Error(_("The package index files are corrupted. No Filename: " - "field for package %s."), - Version.ParentPkg().Name()); + // collect the hashes from the indexes + auto hsl = Parse.Hashes(); + if (ExpectedHashes.empty()) + ExpectedHashes = hsl; + else + { + // bad things will likely happen, but the user might be "lucky" still + // if the sources provide the same hashtypes (so that they aren't mixed) + for (auto const &hs : hsl) + if (ExpectedHashes.push_back(hs) == false) + { + _error->Warning("Sources disagree on hashes for supposely identical version '%s' of '%s'.", + Version.VerStr(), Version.ParentPkg().FullName(false).c_str()); + break; + } + } + // only allow local volatile sources to have no hashes + if (PkgF.Flagged(pkgCache::Flag::LocalSource)) + LocalSource = true; + else if (hsl.empty()) + continue; - Desc.URI = Index->ArchiveURI(PkgFile); - Desc.Description = Index->ArchiveInfo(Version); - Desc.Owner = this; - Desc.ShortDesc = Version.ParentPkg().FullName(true); + std::string poolfilename = Parse.FileName(); + if (poolfilename.empty()) + continue; - auto fields = ModifyCustomFields(); - if (PkgF->Architecture != 0) - fields.emplace("Target-Architecture", PkgF.Architecture()); + std::remove_reference::type fields; + { + auto const debIndex = dynamic_cast(Index); + if (debIndex != nullptr) + { + auto const IT = debIndex->GetIndexTarget(); + fields.emplace("Target-Repo-URI", IT.Option(IndexTarget::REPO_URI)); + fields.emplace("Target-Release", IT.Option(IndexTarget::RELEASE)); + fields.emplace("Target-Site", IT.Option(IndexTarget::SITE)); + } + } + fields.emplace("Target-Base-URI", Index->ArchiveURI("")); if (PkgF->Component != 0) fields.emplace("Target-Component", PkgF.Component()); auto const RelF = PkgF.ReleaseFile(); @@ -3379,78 +3389,91 @@ bool pkgAcqArchive::QueueNext() if (RelF->Archive != 0) fields.emplace("Target-Suite", RelF.Archive()); } + fields.emplace("Target-Architecture", Version.Arch()); + fields.emplace("Target-Type", flExtension(poolfilename)); - // See if we already have the file. (Legacy filenames) - FileSize = Version->Size; - string FinalFile = _config->FindDir("Dir::Cache::Archives") + flNotDir(PkgFile); - struct stat Buf; - if (stat(FinalFile.c_str(),&Buf) == 0) + if (StoreFilename.empty()) { - // Make sure the size matches - if ((unsigned long long)Buf.st_size == Version->Size) - { - Complete = true; - Local = true; - Status = StatDone; - StoreFilename = DestFile = FinalFile; - return true; - } - - /* Hmm, we have a file and its size does not match, this means it is - an old style mismatched arch */ - RemoveFile("pkgAcqArchive::QueueNext", FinalFile); - } - - // Check it again using the new style output filenames - FinalFile = _config->FindDir("Dir::Cache::Archives") + flNotDir(StoreFilename); - if (stat(FinalFile.c_str(),&Buf) == 0) - { - // Make sure the size matches - if ((unsigned long long)Buf.st_size == Version->Size) - { - Complete = true; - Local = true; - Status = StatDone; - StoreFilename = DestFile = FinalFile; - return true; - } - - /* Hmm, we have a file and its size does not match, this shouldn't - happen.. */ - RemoveFile("pkgAcqArchive::QueueNext", FinalFile); - } - - DestFile = _config->FindDir("Dir::Cache::Archives") + "partial/" + flNotDir(StoreFilename); - - // Check the destination file - if (stat(DestFile.c_str(),&Buf) == 0) - { - // Hmm, the partial file is too big, erase it - if ((unsigned long long)Buf.st_size > Version->Size) - RemoveFile("pkgAcqArchive::QueueNext", DestFile); - else - PartialSize = Buf.st_size; + /* We pick a filename based on the information we have for the version, + but we don't know what extension such a file should have, so we look + at the filenames used online and assume that they are the same for + all repositories containing this file */ + StoreFilename = QuoteString(Version.ParentPkg().Name(), "_:") + '_' + + QuoteString(Version.VerStr(), "_:") + '_' + + QuoteString(Version.Arch(), "_:.") + + "." + flExtension(poolfilename); + + Desc.URI = Index->ArchiveURI(poolfilename); + Desc.Description = Index->ArchiveInfo(Version); + Desc.Owner = this; + Desc.ShortDesc = Version.ParentPkg().FullName(true); + auto &customfields = ModifyCustomFields(); + for (auto const &f : fields) + customfields[f.first] = f.second; + FileSize = Version->Size; } + else + PushAlternativeURI(Index->ArchiveURI(poolfilename), std::move(fields), true); + } + if (StoreFilename.empty()) + { + _error->Error(_("Can't find a source to download version '%s' of '%s'"), + Version.VerStr(), Version.ParentPkg().FullName(false).c_str()); + return; + } - // Disables download of archives - useful if no real installation follows, - // e.g. if we are just interested in proposed installation order - if (_config->FindB("Debug::pkgAcqArchive::NoQueue", false) == true) + // Check if we already downloaded the file + struct stat Buf; + auto FinalFile = _config->FindDir("Dir::Cache::Archives") + flNotDir(StoreFilename); + if (stat(FinalFile.c_str(), &Buf) == 0) + { + // Make sure the size matches + if ((unsigned long long)Buf.st_size == Version->Size) { Complete = true; Local = true; Status = StatDone; StoreFilename = DestFile = FinalFile; - return true; + return; } - // Create the item - Local = false; - ++Vf; - QueueURI(Desc); - return true; + /* Hmm, we have a file and its size does not match, this shouldn't + happen.. */ + RemoveFile("pkgAcqArchive::QueueNext", FinalFile); } + + // Check the destination file + DestFile = _config->FindDir("Dir::Cache::Archives") + "partial/" + flNotDir(StoreFilename); + if (stat(DestFile.c_str(), &Buf) == 0) + { + // Hmm, the partial file is too big, erase it + if ((unsigned long long)Buf.st_size > Version->Size) + RemoveFile("pkgAcqArchive::QueueNext", DestFile); + else + PartialSize = Buf.st_size; + } + + // Disables download of archives - useful if no real installation follows, + // e.g. if we are just interested in proposed installation order + if (_config->FindB("Debug::pkgAcqArchive::NoQueue", false) == true) + { + Complete = true; + Local = true; + Status = StatDone; + StoreFilename = DestFile = FinalFile; + return; + } + + // Create the item + Local = false; + QueueURI(Desc); +} +APT_IGNORE_DEPRECATED_POP + /*}}}*/ +bool pkgAcqArchive::QueueNext() /*{{{*/ +{ return false; -} +} /*}}}*/ // AcqArchive::Done - Finished fetching /*{{{*/ // --------------------------------------------------------------------- @@ -3483,25 +3506,6 @@ void pkgAcqArchive::Done(string const &Message, HashStringList const &Hashes, void pkgAcqArchive::Failed(string const &Message,pkgAcquire::MethodConfig const * const Cnf) { Item::Failed(Message,Cnf); - - /* We don't really want to retry on failed media swaps, this prevents - that. An interesting observation is that permanent failures are not - recorded. */ - if (Cnf->Removable == true && - StringToBool(LookupTag(Message,"Transient-Failure"),false) == true) - { - // Vf = Version.FileList(); - while (Vf.end() == false) ++Vf; - StoreFilename = string(); - return; - } - - Status = StatIdle; - if (QueueNext() == false) - { - StoreFilename = string(); - Status = StatError; - } } /*}}}*/ APT_PURE bool pkgAcqArchive::IsTrusted() const /*{{{*/ diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index 7f5f75195..cf227d1b5 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -244,6 +244,9 @@ class pkgAcquire::Item : public WeakPointable /*{{{*/ APT_HIDDEN unsigned int &ModifyRetries(); // this is more a hack than a proper external interface, hence hidden APT_HIDDEN std::unordered_map &ModifyCustomFields(); + // this isn't the super nicest interface either… + APT_HIDDEN bool PopAlternativeURI(std::string &NewURI); + APT_HIDDEN void PushAlternativeURI(std::string &&NewURI, std::unordered_map &&fields, bool const at_the_back); /** \brief A "descriptive" URI-like string. * @@ -993,6 +996,7 @@ class pkgAcqArchive : public pkgAcquire::Item std::string &StoreFilename; /** \brief The next file for this version to try to download. */ + APT_DEPRECATED_MSG("Unused member") pkgCache::VerFileIterator Vf; /** \brief How many (more) times to try to find a new source from diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc index a763d9242..995750dea 100644 --- a/apt-pkg/acquire-worker.cc +++ b/apt-pkg/acquire-worker.cc @@ -211,6 +211,39 @@ static bool isDoomedItem(pkgAcquire::Item const * const Itm) return false; return TransItm->TransactionManager->State != pkgAcqTransactionItem::TransactionStarted; } +static HashStringList GetHashesFromMessage(std::string const &Prefix, std::string const &Message) +{ + HashStringList hsl; + for (char const *const *type = HashString::SupportedHashes(); *type != NULL; ++type) + { + std::string const tagname = Prefix + *type + "-Hash"; + std::string const hashsum = LookupTag(Message, tagname.c_str()); + if (hashsum.empty() == false) + hsl.push_back(HashString(*type, hashsum)); + } + return hsl; +} +static void APT_NONNULL(3) ChangeSiteIsMirrorChange(std::string const &NewURI, pkgAcquire::ItemDesc &desc, pkgAcquire::Item *const Owner) +{ + if (URI::SiteOnly(NewURI) == URI::SiteOnly(desc.URI)) + return; + + auto const firstSpace = desc.Description.find(" "); + if (firstSpace != std::string::npos) + { + std::string const OldSite = desc.Description.substr(0, firstSpace); + if (likely(APT::String::Startswith(desc.URI, OldSite))) + { + std::string const OldExtra = desc.URI.substr(OldSite.length() + 1); + if (likely(APT::String::Endswith(NewURI, OldExtra))) + { + std::string const NewSite = NewURI.substr(0, NewURI.length() - OldExtra.length()); + Owner->UsedMirror = URI::ArchiveOnly(NewSite); + desc.Description.replace(0, firstSpace, Owner->UsedMirror); + } + } + } +} bool pkgAcquire::Worker::RunMessages() { while (MessageQueue.empty() == false) @@ -377,13 +410,7 @@ bool pkgAcquire::Worker::RunMessages() std::string const givenfilename = LookupTag(Message, "Filename"); std::string const filename = givenfilename.empty() ? Itm->Owner->DestFile : givenfilename; // see if we got hashes to verify - for (char const * const * type = HashString::SupportedHashes(); *type != NULL; ++type) - { - std::string const tagname = std::string(*type) + "-Hash"; - std::string const hashsum = LookupTag(Message, tagname.c_str()); - if (hashsum.empty() == false) - ReceivedHashes.push_back(HashString(*type, hashsum)); - } + ReceivedHashes = GetHashesFromMessage("", Message); // not all methods always sent Hashes our way if (ReceivedHashes.usable() == false) { @@ -525,6 +552,7 @@ bool pkgAcquire::Worker::RunMessages() for (auto const Owner: ItmOwners) { + std::string NewURI; if (errTransient == true && Config->LocalOnly == false && Owner->ModifyRetries() != 0) { --Owner->ModifyRetries(); @@ -535,6 +563,17 @@ bool pkgAcquire::Worker::RunMessages() 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) diff --git a/apt-pkg/indexfile.cc b/apt-pkg/indexfile.cc index 3ac5b3679..0e205a562 100644 --- a/apt-pkg/indexfile.cc +++ b/apt-pkg/indexfile.cc @@ -273,6 +273,10 @@ std::string pkgDebianIndexTargetFile::GetProgressDescription() const { return Target.Description; } +IndexTarget pkgDebianIndexTargetFile::GetIndexTarget() const +{ + return Target; +} pkgDebianIndexRealFile::pkgDebianIndexRealFile(std::string const &pFile, bool const Trusted) :/*{{{*/ pkgDebianIndexFile(Trusted), d(NULL) diff --git a/apt-pkg/indexfile.h b/apt-pkg/indexfile.h index 10b15fde4..7ebbd66f1 100644 --- a/apt-pkg/indexfile.h +++ b/apt-pkg/indexfile.h @@ -199,6 +199,7 @@ public: virtual std::string Describe(bool const Short = false) const APT_OVERRIDE; virtual bool Exists() const APT_OVERRIDE; virtual unsigned long Size() const APT_OVERRIDE; + IndexTarget GetIndexTarget() const APT_HIDDEN; pkgDebianIndexTargetFile(IndexTarget const &Target, bool const Trusted); virtual ~pkgDebianIndexTargetFile(); diff --git a/test/integration/framework b/test/integration/framework index bff6c0e65..f9d98835c 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -1298,7 +1298,7 @@ webserverconfig() { rewritesourceslist() { local APTARCHIVE="file://$(readlink -f "${TMPWORKINGDIRECTORY}/aptarchive" | sed 's# #%20#g')" local APTARCHIVE2="copy://$(readlink -f "${TMPWORKINGDIRECTORY}/aptarchive" | sed 's# #%20#g')" - for LIST in $(find rootdir/etc/apt/sources.list.d/ -name 'apt-test-*.list'); do + for LIST in $(find "${TMPWORKINGDIRECTORY}/rootdir/etc/apt/sources.list.d/" -name 'apt-test-*.list'); do sed -i $LIST -e "s#$APTARCHIVE#${1}#" -e "s#$APTARCHIVE2#${1}#" \ -e "s#http://[^@]*@\?localhost:${APTHTTPPORT}/\?#${1}#" \ -e "s#https://[^@]*@\?localhost:${APTHTTPSPORT}/\?#${1}#" diff --git a/test/integration/test-bug-869859-retry-downloads b/test/integration/test-bug-869859-retry-downloads index a62429a53..86203f794 100755 --- a/test/integration/test-bug-869859-retry-downloads +++ b/test/integration/test-bug-869859-retry-downloads @@ -35,3 +35,21 @@ webserverconfig 'aptwebserver::failrequest' '404' webserverconfig 'aptwebserver::failrequest::pool/testpkg_1_all.deb' '2' testfailure apt download testpkg -o acquire::retries=3 testfailure test -f testpkg_1_all.deb + +cat ../rootdir/etc/apt/sources.list.d/apt-test-*.list > ../rootdir/etc/apt/sources.list.d/00http-source.list +changetohttpswebserver + +msgmsg 'Check download from alternative sources if first failed' +webserverconfig 'aptwebserver::failrequest::pool/testpkg_1_all.deb' '0' +testsuccess apt update +testsuccess apt download testpkg -o acquire::retries=0 +testsuccess test -f testpkg_1_all.deb +rm -f testpkg_1_all.deb + +# we make the first source fail by disabling http support +webserverconfig 'aptwebserver::support::http' 'false' +testsuccess apt download testpkg -o acquire::retries=0 +cp ../rootdir/tmp/testsuccess.output alt.output +testsuccess grep '^ 400 Bad Request' alt.output +testsuccess test -f testpkg_1_all.deb +rm -f testpkg_1_all.deb -- cgit v1.2.3-70-g09d2