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. --- apt-pkg/contrib/strutl.cc | 92 +++++++++++++++++++++++++++++++++------------ test/libapt/strutil_test.cc | 42 +++++++++++++++++++++ 2 files changed, 111 insertions(+), 23 deletions(-) diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index f2fc0b6a6..638f09e9d 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -694,34 +694,80 @@ int stringcasecmp(string::const_iterator A,string::const_iterator AEnd, /*}}}*/ // LookupTag - Lookup the value of a tag in a taged string /*{{{*/ // --------------------------------------------------------------------- -/* The format is like those used in package files and the method +/* The format is like those used in package files and the method communication system */ -string LookupTag(const string &Message,const char *Tag,const char *Default) +std::string LookupTag(const std::string &Message, const char *TagC, const char *Default) { - // Look for a matching tag. - int Length = strlen(Tag); - for (string::const_iterator I = Message.begin(); I + Length < Message.end(); ++I) + std::string tag = std::string("\n") + TagC + ":"; + if (Default == nullptr) + Default = ""; + if (Message.length() < tag.length()) + return Default; + std::transform(tag.begin(), tag.end(), tag.begin(), tolower_ascii); + auto valuestart = Message.cbegin(); + // maybe the message starts directly with tag + if (Message[tag.length() - 2] == ':') { - // Found the tag - if (I[Length] == ':' && stringcasecmp(I,I+Length,Tag) == 0) + std::string lowstart = std::string("\n") + Message.substr(0, tag.length() - 1); + std::transform(lowstart.begin(), lowstart.end(), lowstart.begin(), tolower_ascii); + if (lowstart == tag) + valuestart = std::next(valuestart, tag.length() - 1); + } + // the tag is somewhere in the message + if (valuestart == Message.cbegin()) + { + auto const tagbegin = std::search(Message.cbegin(), Message.cend(), tag.cbegin(), tag.cend(), + [](char const a, char const b) { return tolower_ascii(a) == b; }); + if (tagbegin == Message.cend()) + return Default; + valuestart = std::next(tagbegin, tag.length()); + } + auto const is_whitespace = [](char const c) { return isspace_ascii(c) != 0 && c != '\n'; }; + auto const is_newline = [](char const c) { return c == '\n'; }; + std::string result; + valuestart = std::find_if_not(valuestart, Message.cend(), is_whitespace); + // is the first line of the value empty? + if (valuestart != Message.cend() && *valuestart == '\n') + { + valuestart = std::next(valuestart); + if (valuestart != Message.cend() && *valuestart == ' ') + valuestart = std::next(valuestart); + } + // extract the value over multiple lines removing trailing whitespace + while (valuestart < Message.cend()) + { + auto const linebreak = std::find_if(valuestart, Message.cend(), is_newline); + auto valueend = std::prev(linebreak); + // skip spaces at the end of the line + while (valueend > valuestart && is_whitespace(*valueend)) + valueend = std::prev(valueend); + // append found line to result { - // Find the end of line and strip the leading/trailing spaces - string::const_iterator J; - I += Length + 1; - for (; isspace_ascii(*I) != 0 && I < Message.end(); ++I); - for (J = I; *J != '\n' && J < Message.end(); ++J); - for (; J > I && isspace_ascii(J[-1]) != 0; --J); - - return string(I,J); + std::string tmp(valuestart, std::next(valueend)); + if (tmp != ".") + { + if (result.empty()) + result.assign(std::move(tmp)); + else + result.append(tmp); + } } - - for (; *I != '\n' && I < Message.end(); ++I); - } - - // Failed to find a match - if (Default == 0) - return string(); - return Default; + // see if the value is multiline + if (linebreak == Message.cend()) + break; + valuestart = std::next(linebreak); + if (valuestart == Message.cend() || *valuestart != ' ') + break; + result.append("\n"); + // skip the space leading a multiline (Keep all other whitespaces in the value) + valuestart = std::next(valuestart); + } + auto const valueend = result.find_last_not_of("\n"); + if (valueend == std::string::npos) + result.clear(); + else + result.erase(valueend + 1); + return result; } /*}}}*/ // StringToBool - Converts a string into a boolean /*{{{*/ 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 From 2f6aed72f656494d668918aa8ce4052d7c81e993 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 25 Oct 2017 21:40:56 +0200 Subject: mark some 500 HTTP codes as transient acquire errors If retries are enabled only transient errors are retried, which are very few errors. At least for some HTTP codes it could be beneficial to retry them through so adding them seems like a good idea if only to be more consistent in what we report. --- methods/basehttp.cc | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/methods/basehttp.cc b/methods/basehttp.cc index c77ab28c6..3b4c60720 100644 --- a/methods/basehttp.cc +++ b/methods/basehttp.cc @@ -801,7 +801,19 @@ int BaseHttpMethod::Loop() case ERROR_WITH_CONTENT_PAGE: { Server->RunDataToDevNull(Req); - Fail(); + constexpr unsigned int TransientCodes[] = { + 408, // Request Timeout + 429, // Too Many Requests + 500, // Internal Server Error + 502, // Bad Gateway + 503, // Service Unavailable + 504, // Gateway Timeout + 599, // Network Connect Timeout Error + }; + if (std::find(std::begin(TransientCodes), std::end(TransientCodes), Req.Result) != std::end(TransientCodes)) + Fail(true); + else + Fail(); break; } -- cgit v1.2.3 From 47c0bdc310c8cd62374ca6e6bb456dd183bdfc07 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 26 Oct 2017 00:57:26 +0200 Subject: report transient errors as transient errors The Fail method for acquire methods has a boolean parameter indicating the transient-nature of a reported error. The problem with this is that Fail is called very late at a point where it is no longer easily identifiable if an error is indeed transient or not, so some calls were and some weren't and the acquire system would later mostly ignore the transient flag and guess by using the FailReason instead. Introducing a tri-state enum we can pass the information about fatal or transient errors through the callstack to generate the correct fails. --- methods/aptmethod.h | 7 ++ methods/basehttp.cc | 45 ++++++--- methods/basehttp.h | 13 ++- methods/connect.cc | 261 ++++++++++++++++++++++++++++++++++++---------------- methods/connect.h | 10 +- methods/ftp.cc | 108 ++++++++++++++-------- methods/ftp.h | 4 +- methods/http.cc | 203 ++++++++++++++++++++++++---------------- methods/http.h | 14 ++- 9 files changed, 433 insertions(+), 232 deletions(-) diff --git a/methods/aptmethod.h b/methods/aptmethod.h index 8d37cbe80..88d325cba 100644 --- a/methods/aptmethod.h +++ b/methods/aptmethod.h @@ -29,6 +29,13 @@ #include #endif +enum class ResultState +{ + TRANSIENT_ERROR, + FATAL_ERROR, + SUCCESSFUL +}; + static bool hasDoubleColon(std::string const &n) { return n.find("::") != std::string::npos; diff --git a/methods/basehttp.cc b/methods/basehttp.cc index 3b4c60720..2473ecb5c 100644 --- a/methods/basehttp.cc +++ b/methods/basehttp.cc @@ -74,9 +74,8 @@ ServerState::RunHeadersResult ServerState::RunHeaders(RequestState &Req, Persistent = false; return RUN_HEADERS_OK; - } - while (LoadNextResponse(false, Req) == true); - + } while (LoadNextResponse(false, Req) == ResultState::SUCCESSFUL); + return RUN_HEADERS_IO_ERROR; } /*}}}*/ @@ -598,11 +597,18 @@ int BaseHttpMethod::Loop() QueueBack = Queue; // Connect to the host - if (Server->Open() == false) + switch (Server->Open()) { + case ResultState::FATAL_ERROR: + Fail(false); + Server = nullptr; + continue; + case ResultState::TRANSIENT_ERROR: Fail(true); Server = nullptr; continue; + case ResultState::SUCCESSFUL: + break; } // Fill the pipeline. @@ -657,13 +663,13 @@ int BaseHttpMethod::Loop() URIStart(Res); // Run the data - bool Result = true; + ResultState Result = ResultState::SUCCESSFUL; - // ensure we don't fetch too much - // we could do "Server->MaximumSize = Queue->MaximumSize" here - // but that would break the clever pipeline messup detection - // so instead we use the size of the biggest item in the queue - Req.MaximumSize = FindMaximumObjectSizeInQueue(); + // ensure we don't fetch too much + // we could do "Server->MaximumSize = Queue->MaximumSize" here + // but that would break the clever pipeline messup detection + // so instead we use the size of the biggest item in the queue + Req.MaximumSize = FindMaximumObjectSizeInQueue(); if (Req.HaveContent) { @@ -692,10 +698,10 @@ int BaseHttpMethod::Loop() SetFailReason("MaximumSizeExceeded"); _error->Error(_("File has unexpected size (%llu != %llu). Mirror sync in progress?"), filesize, Queue->ExpectedHashes.FileSize()); - Result = false; + Result = ResultState::FATAL_ERROR; } } - if (Result) + if (Result == ResultState::SUCCESSFUL) Result = Server->RunData(Req); } @@ -715,7 +721,7 @@ int BaseHttpMethod::Loop() utimes(Queue->DestFile.c_str(), times); // Send status to APT - if (Result == true) + if (Result == ResultState::SUCCESSFUL) { Hashes * const resultHashes = Server->GetHashes(); HashStringList const hashList = resultHashes->GetHashStringList(); @@ -768,8 +774,17 @@ int BaseHttpMethod::Loop() else { Server->Close(); - Fail(true); - } + switch (Result) + { + case ResultState::TRANSIENT_ERROR: + Fail(true); + break; + case ResultState::FATAL_ERROR: + case ResultState::SUCCESSFUL: + Fail(false); + break; + } + } } break; } diff --git a/methods/basehttp.h b/methods/basehttp.h index aadd59168..8220c1b3c 100644 --- a/methods/basehttp.h +++ b/methods/basehttp.h @@ -62,7 +62,6 @@ struct RequestState RequestState(BaseHttpMethod * const Owner, ServerState * const Server) : Owner(Owner), Server(Server) { time(&Date); } }; - struct ServerState { bool Persistent; @@ -78,7 +77,7 @@ struct ServerState BaseHttpMethod *Owner; virtual bool ReadHeaderLines(std::string &Data) = 0; - virtual bool LoadNextResponse(bool const ToFile, RequestState &Req) = 0; + virtual ResultState LoadNextResponse(bool const ToFile, RequestState &Req) = 0; public: @@ -99,16 +98,16 @@ struct ServerState virtual bool WriteResponse(std::string const &Data) = 0; /** \brief Transfer the data from the socket */ - virtual bool RunData(RequestState &Req) = 0; - virtual bool RunDataToDevNull(RequestState &Req) = 0; + virtual ResultState RunData(RequestState &Req) = 0; + virtual ResultState RunDataToDevNull(RequestState &Req) = 0; - virtual bool Open() = 0; + virtual ResultState Open() = 0; virtual bool IsOpen() = 0; virtual bool Close() = 0; virtual bool InitHashes(HashStringList const &ExpectedHashes) = 0; - virtual bool Die(RequestState &Req) = 0; + virtual ResultState Die(RequestState &Req) = 0; virtual bool Flush(FileFd * const File) = 0; - virtual bool Go(bool ToFile, RequestState &Req) = 0; + virtual ResultState Go(bool ToFile, RequestState &Req) = 0; virtual Hashes * GetHashes() = 0; ServerState(URI Srv, BaseHttpMethod *Owner); diff --git a/methods/connect.cc b/methods/connect.cc index 6a7b71c0b..1354fe97b 100644 --- a/methods/connect.cc +++ b/methods/connect.cc @@ -112,8 +112,8 @@ std::unique_ptr MethodFd::FromFd(int iFd) // DoConnect - Attempt a connect operation /*{{{*/ // --------------------------------------------------------------------- /* This helper function attempts a connection to a single address. */ -static bool DoConnect(struct addrinfo *Addr, std::string const &Host, - unsigned long TimeOut, std::unique_ptr &Fd, aptMethod *Owner) +static ResultState DoConnect(struct addrinfo *Addr, std::string const &Host, + unsigned long TimeOut, std::unique_ptr &Fd, aptMethod *Owner) { // Show a status indicator char Name[NI_MAXHOST]; @@ -129,7 +129,7 @@ static bool DoConnect(struct addrinfo *Addr, std::string const &Host, // if that addr did timeout before, we do not try it again if(bad_addr.find(std::string(Name)) != bad_addr.end()) - return false; + return ResultState::TRANSIENT_ERROR; /* If this is an IP rotation store the IP we are using.. If something goes wrong this will get tacked onto the end of the error message */ @@ -143,31 +143,43 @@ static bool DoConnect(struct addrinfo *Addr, std::string const &Host, // Get a socket if ((static_cast(Fd.get())->fd = socket(Addr->ai_family, Addr->ai_socktype, Addr->ai_protocol)) < 0) - return _error->Errno("socket",_("Could not create a socket for %s (f=%u t=%u p=%u)"), - Name,Addr->ai_family,Addr->ai_socktype,Addr->ai_protocol); + { + _error->Errno("socket", _("Could not create a socket for %s (f=%u t=%u p=%u)"), + Name, Addr->ai_family, Addr->ai_socktype, Addr->ai_protocol); + return ResultState::FATAL_ERROR; + } SetNonBlock(Fd->Fd(), true); if (connect(Fd->Fd(), Addr->ai_addr, Addr->ai_addrlen) < 0 && errno != EINPROGRESS) - return _error->Errno("connect",_("Cannot initiate the connection " - "to %s:%s (%s)."),Host.c_str(),Service,Name); - + { + _error->Errno("connect", _("Cannot initiate the connection " + "to %s:%s (%s)."), + Host.c_str(), Service, Name); + return ResultState::TRANSIENT_ERROR; + } + /* This implements a timeout for connect by opening the connection nonblocking */ if (WaitFd(Fd->Fd(), true, TimeOut) == false) { bad_addr.insert(bad_addr.begin(), std::string(Name)); Owner->SetFailReason("Timeout"); - return _error->Error(_("Could not connect to %s:%s (%s), " - "connection timed out"),Host.c_str(),Service,Name); + _error->Error(_("Could not connect to %s:%s (%s), " + "connection timed out"), + Host.c_str(), Service, Name); + return ResultState::TRANSIENT_ERROR; } // Check the socket for an error condition unsigned int Err; unsigned int Len = sizeof(Err); if (getsockopt(Fd->Fd(), SOL_SOCKET, SO_ERROR, &Err, &Len) != 0) - return _error->Errno("getsockopt",_("Failed")); - + { + _error->Errno("getsockopt", _("Failed")); + return ResultState::FATAL_ERROR; + } + if (Err != 0) { errno = Err; @@ -176,22 +188,23 @@ static bool DoConnect(struct addrinfo *Addr, std::string const &Host, else if (errno == ETIMEDOUT) Owner->SetFailReason("ConnectionTimedOut"); bad_addr.insert(bad_addr.begin(), std::string(Name)); - return _error->Errno("connect",_("Could not connect to %s:%s (%s)."),Host.c_str(), - Service,Name); + _error->Errno("connect", _("Could not connect to %s:%s (%s)."), Host.c_str(), + Service, Name); + return ResultState::TRANSIENT_ERROR; } Owner->SetFailReason(""); - return true; + return ResultState::SUCCESSFUL; } /*}}}*/ // Connect to a given Hostname /*{{{*/ -static bool ConnectToHostname(std::string const &Host, int const Port, - const char *const Service, int DefPort, std::unique_ptr &Fd, - unsigned long const TimeOut, aptMethod *const Owner) +static ResultState ConnectToHostname(std::string const &Host, int const Port, + const char *const Service, int DefPort, std::unique_ptr &Fd, + unsigned long const TimeOut, aptMethod *const Owner) { if (ConnectionAllowed(Service, Host) == false) - return false; + return ResultState::FATAL_ERROR; // Convert the port name/number char ServStr[300]; if (Port != 0) @@ -238,8 +251,11 @@ static bool ConnectToHostname(std::string const &Host, int const Port, Hints.ai_family = AF_UNSPEC; // if we couldn't resolve the host before, we don't try now - if(bad_addr.find(Host) != bad_addr.end()) - return _error->Error(_("Could not resolve '%s'"),Host.c_str()); + if (bad_addr.find(Host) != bad_addr.end()) + { + _error->Error(_("Could not resolve '%s'"), Host.c_str()); + return ResultState::TRANSIENT_ERROR; + } // Resolve both the host and service simultaneously while (1) @@ -258,20 +274,24 @@ static bool ConnectToHostname(std::string const &Host, int const Port, } bad_addr.insert(bad_addr.begin(), Host); Owner->SetFailReason("ResolveFailure"); - return _error->Error(_("Could not resolve '%s'"),Host.c_str()); + _error->Error(_("Could not resolve '%s'"), Host.c_str()); + return ResultState::TRANSIENT_ERROR; } if (Res == EAI_AGAIN) { Owner->SetFailReason("TmpResolveFailure"); - return _error->Error(_("Temporary failure resolving '%s'"), - Host.c_str()); + _error->Error(_("Temporary failure resolving '%s'"), + Host.c_str()); + return ResultState::TRANSIENT_ERROR; } if (Res == EAI_SYSTEM) - return _error->Errno("getaddrinfo", _("System error resolving '%s:%s'"), - Host.c_str(),ServStr); - return _error->Error(_("Something wicked happened resolving '%s:%s' (%i - %s)"), - Host.c_str(),ServStr,Res,gai_strerror(Res)); + _error->Errno("getaddrinfo", _("System error resolving '%s:%s'"), + Host.c_str(), ServStr); + else + _error->Error(_("Something wicked happened resolving '%s:%s' (%i - %s)"), + Host.c_str(), ServStr, Res, gai_strerror(Res)); + return ResultState::TRANSIENT_ERROR; } break; } @@ -287,10 +307,11 @@ static bool ConnectToHostname(std::string const &Host, int const Port, while (CurHost != 0) { - if (DoConnect(CurHost,Host,TimeOut,Fd,Owner) == true) + auto const result = DoConnect(CurHost, Host, TimeOut, Fd, Owner); + if (result == ResultState::SUCCESSFUL) { LastUsed = CurHost; - return true; + return result; } Fd->Close(); @@ -315,22 +336,23 @@ static bool ConnectToHostname(std::string const &Host, int const Port, } if (_error->PendingError() == true) - return false; - return _error->Error(_("Unable to connect to %s:%s:"),Host.c_str(),ServStr); + return ResultState::FATAL_ERROR; + _error->Error(_("Unable to connect to %s:%s:"), Host.c_str(), ServStr); + return ResultState::TRANSIENT_ERROR; } /*}}}*/ // Connect - Connect to a server /*{{{*/ // --------------------------------------------------------------------- /* Performs a connection to the server (including SRV record lookup) */ -bool Connect(std::string Host, int Port, const char *Service, - int DefPort, std::unique_ptr &Fd, - unsigned long TimeOut, aptMethod *Owner) +ResultState Connect(std::string Host, int Port, const char *Service, + int DefPort, std::unique_ptr &Fd, + unsigned long TimeOut, aptMethod *Owner) { if (_error->PendingError() == true) - return false; + return ResultState::FATAL_ERROR; if (ConnectionAllowed(Service, Host) == false) - return false; + return ResultState::FATAL_ERROR; if(LastHost != Host || LastPort != Port) { @@ -340,8 +362,12 @@ bool Connect(std::string Host, int Port, const char *Service, GetSrvRecords(Host, DefPort, SrvRecords); // RFC2782 defines that a lonely '.' target is an abort reason if (SrvRecords.size() == 1 && SrvRecords[0].target.empty()) - return _error->Error("SRV records for %s indicate that " - "%s service is not available at this domain", Host.c_str(), Service); + { + _error->Error("SRV records for %s indicate that " + "%s service is not available at this domain", + Host.c_str(), Service); + return ResultState::FATAL_ERROR; + } } } @@ -358,11 +384,11 @@ bool Connect(std::string Host, int Port, const char *Service, Host = Srv.target; Port = Srv.port; auto const ret = ConnectToHostname(Host, Port, Service, DefPort, Fd, TimeOut, Owner); - if (ret) + if (ret == ResultState::SUCCESSFUL) { while(stackSize--) _error->RevertToStack(); - return true; + return ret; } } Host = std::move(initialHost); @@ -374,7 +400,7 @@ bool Connect(std::string Host, int Port, const char *Service, auto const ret = ConnectToHostname(Host, Port, Service, DefPort, Fd, TimeOut, Owner); while(stackSize--) - if (ret) + if (ret == ResultState::SUCCESSFUL) _error->RevertToStack(); else _error->MergeWithStack(); @@ -403,8 +429,8 @@ static bool TalkToSocksProxy(int const ServerFd, std::string const &Proxy, return true; } -bool UnwrapSocks(std::string Host, int Port, URI Proxy, std::unique_ptr &Fd, - unsigned long Timeout, aptMethod *Owner) +ResultState UnwrapSocks(std::string Host, int Port, URI Proxy, std::unique_ptr &Fd, + unsigned long Timeout, aptMethod *Owner) { /* We implement a very basic SOCKS5 client here complying mostly to RFC1928 expect * for not offering GSSAPI auth which is a must (we only do no or user/pass auth). @@ -413,14 +439,20 @@ bool UnwrapSocks(std::string Host, int Port, URI Proxy, std::unique_ptrStatus(_("Connecting to %s (%s)"), "SOCKS5h proxy", ProxyInfo.c_str()); #define APT_WriteOrFail(TYPE, DATA, LENGTH) \ if (TalkToSocksProxy(Fd->Fd(), ProxyInfo, TYPE, true, DATA, LENGTH, Timeout) == false) \ - return false + return ResultState::TRANSIENT_ERROR #define APT_ReadOrFail(TYPE, DATA, LENGTH) \ if (TalkToSocksProxy(Fd->Fd(), ProxyInfo, TYPE, false, DATA, LENGTH, Timeout) == false) \ - return false + return ResultState::TRANSIENT_ERROR if (Host.length() > 255) - return _error->Error("Can't use SOCKS5h as hostname %s is too long!", Host.c_str()); + { + _error->Error("Can't use SOCKS5h as hostname %s is too long!", Host.c_str()); + return ResultState::FATAL_ERROR; + } if (Proxy.User.length() > 255 || Proxy.Password.length() > 255) - return _error->Error("Can't use user&pass auth as they are too long (%lu and %lu) for the SOCKS5!", Proxy.User.length(), Proxy.Password.length()); + { + _error->Error("Can't use user&pass auth as they are too long (%lu and %lu) for the SOCKS5!", Proxy.User.length(), Proxy.Password.length()); + return ResultState::FATAL_ERROR; + } if (Proxy.User.empty()) { uint8_t greeting[] = {0x05, 0x01, 0x00}; @@ -434,13 +466,19 @@ bool UnwrapSocks(std::string Host, int Port, URI Proxy, std::unique_ptrError("SOCKS proxy %s greets back with wrong version: %d", ProxyInfo.c_str(), greeting[0]); + { + _error->Error("SOCKS proxy %s greets back with wrong version: %d", ProxyInfo.c_str(), greeting[0]); + return ResultState::FATAL_ERROR; + } if (greeting[1] == 0x00) ; // no auth has no method-dependent sub-negotiations else if (greeting[1] == 0x02) { if (Proxy.User.empty()) - return _error->Error("SOCKS proxy %s negotiated user&pass auth, but we had not offered it!", ProxyInfo.c_str()); + { + _error->Error("SOCKS proxy %s negotiated user&pass auth, but we had not offered it!", ProxyInfo.c_str()); + return ResultState::FATAL_ERROR; + } // user&pass auth sub-negotiations are defined by RFC1929 std::vector auth = {{0x01, static_cast(Proxy.User.length())}}; std::copy(Proxy.User.begin(), Proxy.User.end(), std::back_inserter(auth)); @@ -450,12 +488,21 @@ bool UnwrapSocks(std::string Host, int Port, URI Proxy, std::unique_ptrError("SOCKS proxy %s auth status response with wrong version: %d", ProxyInfo.c_str(), authstatus[0]); + { + _error->Error("SOCKS proxy %s auth status response with wrong version: %d", ProxyInfo.c_str(), authstatus[0]); + return ResultState::FATAL_ERROR; + } if (authstatus[1] != 0x00) - return _error->Error("SOCKS proxy %s reported authorization failure: username or password incorrect? (%d)", ProxyInfo.c_str(), authstatus[1]); + { + _error->Error("SOCKS proxy %s reported authorization failure: username or password incorrect? (%d)", ProxyInfo.c_str(), authstatus[1]); + return ResultState::FATAL_ERROR; + } } else - return _error->Error("SOCKS proxy %s greets back having not found a common authorization method: %d", ProxyInfo.c_str(), greeting[1]); + { + _error->Error("SOCKS proxy %s greets back having not found a common authorization method: %d", ProxyInfo.c_str(), greeting[1]); + return ResultState::FATAL_ERROR; + } union { uint16_t *i; uint8_t *b; @@ -470,9 +517,15 @@ bool UnwrapSocks(std::string Host, int Port, URI Proxy, std::unique_ptrError("SOCKS proxy %s response with wrong version: %d", ProxyInfo.c_str(), response[0]); + { + _error->Error("SOCKS proxy %s response with wrong version: %d", ProxyInfo.c_str(), response[0]); + return ResultState::FATAL_ERROR; + } if (response[2] != 0x00) - return _error->Error("SOCKS proxy %s has unexpected non-zero reserved field value: %d", ProxyInfo.c_str(), response[2]); + { + _error->Error("SOCKS proxy %s has unexpected non-zero reserved field value: %d", ProxyInfo.c_str(), response[2]); + return ResultState::FATAL_ERROR; + } std::string bindaddr; if (response[3] == 0x01) // IPv4 address { @@ -508,12 +561,16 @@ bool UnwrapSocks(std::string Host, int Port, URI Proxy, std::unique_ptrError("SOCKS proxy %s destination address is of unknown type: %d", - ProxyInfo.c_str(), response[3]); + { + _error->Error("SOCKS proxy %s destination address is of unknown type: %d", + ProxyInfo.c_str(), response[3]); + return ResultState::FATAL_ERROR; + } if (response[1] != 0x00) { char const *errstr = nullptr; auto errcode = response[1]; + bool Transient = false; // Tor error reporting can be a bit arcane, lets try to detect & fix it up if (bindaddr == "0.0.0.0:0") { @@ -554,18 +611,22 @@ bool UnwrapSocks(std::string Host, int Port, URI Proxy, std::unique_ptrSetFailReason("ConnectionTimedOut"); + Transient = true; break; case 0x04: errstr = "Host unreachable"; Owner->SetFailReason("ConnectionTimedOut"); + Transient = true; break; case 0x05: errstr = "Connection refused"; Owner->SetFailReason("ConnectionRefused"); + Transient = true; break; case 0x06: errstr = "TTL expired"; Owner->SetFailReason("Timeout"); + Transient = true; break; case 0x07: errstr = "Command not supported"; @@ -581,20 +642,24 @@ bool UnwrapSocks(std::string Host, int Port, URI Proxy, std::unique_ptrError("SOCKS proxy %s could not connect to %s (%s) due to: %s (%d)", - ProxyInfo.c_str(), Host.c_str(), bindaddr.c_str(), errstr, response[1]); + _error->Error("SOCKS proxy %s could not connect to %s (%s) due to: %s (%d)", + ProxyInfo.c_str(), Host.c_str(), bindaddr.c_str(), errstr, response[1]); + return Transient ? ResultState::TRANSIENT_ERROR : ResultState::FATAL_ERROR; } else if (Owner->DebugEnabled()) ioprintf(std::clog, "http: SOCKS proxy %s connection established to %s (%s)\n", ProxyInfo.c_str(), Host.c_str(), bindaddr.c_str()); if (WaitFd(Fd->Fd(), true, Timeout) == false) - return _error->Error("SOCKS proxy %s reported connection to %s (%s), but timed out", - ProxyInfo.c_str(), Host.c_str(), bindaddr.c_str()); + { + _error->Error("SOCKS proxy %s reported connection to %s (%s), but timed out", + ProxyInfo.c_str(), Host.c_str(), bindaddr.c_str()); + return ResultState::TRANSIENT_ERROR; + } #undef APT_ReadOrFail #undef APT_WriteOrFail - return true; + return ResultState::SUCCESSFUL; } /*}}}*/ // UnwrapTLS - Handle TLS connections /*{{{*/ @@ -643,11 +708,14 @@ struct TlsFd : public MethodFd } }; -bool UnwrapTLS(std::string Host, std::unique_ptr &Fd, - unsigned long Timeout, aptMethod *Owner) +ResultState UnwrapTLS(std::string Host, std::unique_ptr &Fd, + unsigned long Timeout, aptMethod *Owner) { if (_config->FindB("Acquire::AllowTLS", true) == false) - return _error->Error("TLS support has been disabled: Acquire::AllowTLS is false."); + { + _error->Error("TLS support has been disabled: Acquire::AllowTLS is false."); + return ResultState::FATAL_ERROR; + } int err; TlsFd *tlsFd = new TlsFd(); @@ -656,7 +724,10 @@ bool UnwrapTLS(std::string Host, std::unique_ptr &Fd, tlsFd->UnderlyingFd = MethodFd::FromFd(-1); // For now if ((err = gnutls_init(&tlsFd->session, GNUTLS_CLIENT | GNUTLS_NONBLOCK)) < 0) - return _error->Error("Internal error: could not allocate credentials: %s", gnutls_strerror(err)); + { + _error->Error("Internal error: could not allocate credentials: %s", gnutls_strerror(err)); + return ResultState::FATAL_ERROR; + } FdFd *fdfd = dynamic_cast(Fd.get()); if (fdfd != nullptr) @@ -677,7 +748,10 @@ bool UnwrapTLS(std::string Host, std::unique_ptr &Fd, } if ((err = gnutls_certificate_allocate_credentials(&tlsFd->credentials)) < 0) - return _error->Error("Internal error: could not allocate credentials: %s", gnutls_strerror(err)); + { + _error->Error("Internal error: could not allocate credentials: %s", gnutls_strerror(err)); + return ResultState::FATAL_ERROR; + } // Credential setup std::string fileinfo = Owner->ConfigFind("CaInfo", ""); @@ -688,7 +762,10 @@ bool UnwrapTLS(std::string Host, std::unique_ptr &Fd, if (err == 0) Owner->Warning("No system certificates available. Try installing ca-certificates."); else if (err < 0) - return _error->Error("Could not load system TLS certificates: %s", gnutls_strerror(err)); + { + _error->Error("Could not load system TLS certificates: %s", gnutls_strerror(err)); + return ResultState::FATAL_ERROR; + } } else { @@ -696,13 +773,22 @@ bool UnwrapTLS(std::string Host, std::unique_ptr &Fd, gnutls_certificate_set_verify_flags(tlsFd->credentials, GNUTLS_VERIFY_ALLOW_X509_V1_CA_CRT); err = gnutls_certificate_set_x509_trust_file(tlsFd->credentials, fileinfo.c_str(), GNUTLS_X509_FMT_PEM); if (err < 0) - return _error->Error("Could not load certificates from %s (CaInfo option): %s", fileinfo.c_str(), gnutls_strerror(err)); + { + _error->Error("Could not load certificates from %s (CaInfo option): %s", fileinfo.c_str(), gnutls_strerror(err)); + return ResultState::FATAL_ERROR; + } } if (!Owner->ConfigFind("IssuerCert", "").empty()) - return _error->Error("The option '%s' is not supported anymore", "IssuerCert"); + { + _error->Error("The option '%s' is not supported anymore", "IssuerCert"); + return ResultState::FATAL_ERROR; + } if (!Owner->ConfigFind("SslForceVersion", "").empty()) - return _error->Error("The option '%s' is not supported anymore", "SslForceVersion"); + { + _error->Error("The option '%s' is not supported anymore", "SslForceVersion"); + return ResultState::FATAL_ERROR; + } // For client authentication, certificate file ... std::string const cert = Owner->ConfigFind("SslCert", ""); @@ -715,7 +801,8 @@ bool UnwrapTLS(std::string Host, std::unique_ptr &Fd, key.empty() ? cert.c_str() : key.c_str(), GNUTLS_X509_FMT_PEM)) < 0) { - return _error->Error("Could not load client certificate (%s, SslCert option) or key (%s, SslKey option): %s", cert.c_str(), key.c_str(), gnutls_strerror(err)); + _error->Error("Could not load client certificate (%s, SslCert option) or key (%s, SslKey option): %s", cert.c_str(), key.c_str(), gnutls_strerror(err)); + return ResultState::FATAL_ERROR; } } @@ -726,14 +813,23 @@ bool UnwrapTLS(std::string Host, std::unique_ptr &Fd, if ((err = gnutls_certificate_set_x509_crl_file(tlsFd->credentials, crlfile.c_str(), GNUTLS_X509_FMT_PEM)) < 0) - return _error->Error("Could not load custom certificate revocation list %s (CrlFile option): %s", crlfile.c_str(), gnutls_strerror(err)); + { + _error->Error("Could not load custom certificate revocation list %s (CrlFile option): %s", crlfile.c_str(), gnutls_strerror(err)); + return ResultState::FATAL_ERROR; + } } if ((err = gnutls_credentials_set(tlsFd->session, GNUTLS_CRD_CERTIFICATE, tlsFd->credentials)) < 0) - return _error->Error("Internal error: Could not add certificates to session: %s", gnutls_strerror(err)); + { + _error->Error("Internal error: Could not add certificates to session: %s", gnutls_strerror(err)); + return ResultState::FATAL_ERROR; + } if ((err = gnutls_set_default_priority(tlsFd->session)) < 0) - return _error->Error("Internal error: Could not set algorithm preferences: %s", gnutls_strerror(err)); + { + _error->Error("Internal error: Could not set algorithm preferences: %s", gnutls_strerror(err)); + return ResultState::FATAL_ERROR; + } if (Owner->ConfigFindB("Verify-Peer", true)) { @@ -749,7 +845,10 @@ bool UnwrapTLS(std::string Host, std::unique_ptr &Fd, inet_pton(AF_INET6, tlsFd->hostname.c_str(), &addr6) == 1) /* not a host name */; else if ((err = gnutls_server_name_set(tlsFd->session, GNUTLS_NAME_DNS, tlsFd->hostname.c_str(), tlsFd->hostname.length())) < 0) - return _error->Error("Could not set host name %s to indicate to server: %s", tlsFd->hostname.c_str(), gnutls_strerror(err)); + { + _error->Error("Could not set host name %s to indicate to server: %s", tlsFd->hostname.c_str(), gnutls_strerror(err)); + return ResultState::FATAL_ERROR; + } } // Set the FD now, so closing it works reliably. @@ -763,7 +862,10 @@ bool UnwrapTLS(std::string Host, std::unique_ptr &Fd, err = gnutls_handshake(tlsFd->session); if ((err == GNUTLS_E_INTERRUPTED || err == GNUTLS_E_AGAIN) && WaitFd(Fd->Fd(), gnutls_record_get_direction(tlsFd->session) == 1, Timeout) == false) - return _error->Errno("select", "Could not wait for server fd"); + { + _error->Errno("select", "Could not wait for server fd"); + return ResultState::TRANSIENT_ERROR; + } } while (err < 0 && gnutls_error_is_fatal(err) == 0); if (err < 0) @@ -781,9 +883,10 @@ bool UnwrapTLS(std::string Host, std::unique_ptr &Fd, } gnutls_free(txt.data); } - return _error->Error("Could not handshake: %s", gnutls_strerror(err)); + _error->Error("Could not handshake: %s", gnutls_strerror(err)); + return ResultState::FATAL_ERROR; } - return true; + return ResultState::SUCCESSFUL; } /*}}}*/ diff --git a/methods/connect.h b/methods/connect.h index 817ebf765..63a94400e 100644 --- a/methods/connect.h +++ b/methods/connect.h @@ -14,7 +14,7 @@ #include #include -class aptMethod; +#include "aptmethod.h" /** * \brief Small representation of a file descriptor for network traffic. @@ -39,11 +39,11 @@ struct MethodFd virtual bool HasPending(); }; -bool Connect(std::string To, int Port, const char *Service, int DefPort, - std::unique_ptr &Fd, unsigned long TimeOut, aptMethod *Owner); +ResultState Connect(std::string To, int Port, const char *Service, int DefPort, + std::unique_ptr &Fd, unsigned long TimeOut, aptMethod *Owner); -bool UnwrapSocks(std::string To, int Port, URI Proxy, std::unique_ptr &Fd, unsigned long Timeout, aptMethod *Owner); -bool UnwrapTLS(std::string To, std::unique_ptr &Fd, unsigned long Timeout, aptMethod *Owner); +ResultState UnwrapSocks(std::string To, int Port, URI Proxy, std::unique_ptr &Fd, unsigned long Timeout, aptMethod *Owner); +ResultState UnwrapTLS(std::string To, std::unique_ptr &Fd, unsigned long Timeout, aptMethod *Owner); void RotateDNS(); diff --git a/methods/ftp.cc b/methods/ftp.cc index 99eebc5af..2daba8ffe 100644 --- a/methods/ftp.cc +++ b/methods/ftp.cc @@ -110,12 +110,12 @@ void FTPConn::Close() // --------------------------------------------------------------------- /* Connect to the server using a non-blocking connection and perform a login. */ -bool FTPConn::Open(aptMethod *Owner) +ResultState FTPConn::Open(aptMethod *Owner) { // Use the already open connection if possible. if (ServerFd->Fd() != -1) - return true; - + return ResultState::SUCCESSFUL; + Close(); // Determine the proxy setting @@ -167,31 +167,40 @@ bool FTPConn::Open(aptMethod *Owner) /* Connect to the remote server. Since FTP is connection oriented we want to make sure we get a new server every time we reconnect */ RotateDNS(); - if (Connect(Host,Port,"ftp",21,ServerFd,TimeOut,Owner) == false) - return false; + auto result = Connect(Host, Port, "ftp", 21, ServerFd, TimeOut, Owner); + if (result != ResultState::SUCCESSFUL) + return result; // Login must be before getpeername otherwise dante won't work. Owner->Status(_("Logging in")); - bool Res = Login(); - + result = Login(); + if (result != ResultState::SUCCESSFUL) + return result; + // Get the remote server's address PeerAddrLen = sizeof(PeerAddr); if (getpeername(ServerFd->Fd(), (sockaddr *)&PeerAddr, &PeerAddrLen) != 0) - return _error->Errno("getpeername",_("Unable to determine the peer name")); - + { + _error->Errno("getpeername", _("Unable to determine the peer name")); + return ResultState::TRANSIENT_ERROR; + } + // Get the local machine's address ServerAddrLen = sizeof(ServerAddr); if (getsockname(ServerFd->Fd(), (sockaddr *)&ServerAddr, &ServerAddrLen) != 0) - return _error->Errno("getsockname",_("Unable to determine the local name")); - - return Res; + { + _error->Errno("getsockname", _("Unable to determine the local name")); + return ResultState::TRANSIENT_ERROR; + } + + return ResultState::SUCCESSFUL; } /*}}}*/ // FTPConn::Login - Login to the remote server /*{{{*/ // --------------------------------------------------------------------- /* This performs both normal login and proxy login using a simples script stored in the config file. */ -bool FTPConn::Login() +ResultState FTPConn::Login() { unsigned int Tag; string Msg; @@ -211,22 +220,31 @@ bool FTPConn::Login() { // Read the initial response if (ReadResp(Tag,Msg) == false) - return false; + return ResultState::TRANSIENT_ERROR; if (Tag >= 400) - return _error->Error(_("The server refused the connection and said: %s"),Msg.c_str()); - + { + _error->Error(_("The server refused the connection and said: %s"), Msg.c_str()); + return ResultState::FATAL_ERROR; + } + // Send the user if (WriteMsg(Tag,Msg,"USER %s",User.c_str()) == false) - return false; + return ResultState::TRANSIENT_ERROR; if (Tag >= 400) - return _error->Error(_("USER failed, server said: %s"),Msg.c_str()); - + { + _error->Error(_("USER failed, server said: %s"), Msg.c_str()); + return ResultState::FATAL_ERROR; + } + if (Tag == 331) { // 331 User name okay, need password. // Send the Password if (WriteMsg(Tag,Msg,"PASS %s",Pass.c_str()) == false) - return false; - if (Tag >= 400) - return _error->Error(_("PASS failed, server said: %s"),Msg.c_str()); + return ResultState::TRANSIENT_ERROR; + if (Tag >= 400) + { + _error->Error(_("PASS failed, server said: %s"), Msg.c_str()); + return ResultState::FATAL_ERROR; + } } // Enter passive mode @@ -239,15 +257,21 @@ bool FTPConn::Login() { // Read the initial response if (ReadResp(Tag,Msg) == false) - return false; + return ResultState::TRANSIENT_ERROR; if (Tag >= 400) - return _error->Error(_("The server refused the connection and said: %s"),Msg.c_str()); - + { + _error->Error(_("The server refused the connection and said: %s"), Msg.c_str()); + return ResultState::TRANSIENT_ERROR; + } + // Perform proxy script execution Configuration::Item const *Opts = _config->Tree("Acquire::ftp::ProxyLogin"); if (Opts == 0 || Opts->Child == 0) - return _error->Error(_("A proxy server was specified but no login " - "script, Acquire::ftp::ProxyLogin is empty.")); + { + _error->Error(_("A proxy server was specified but no login " + "script, Acquire::ftp::ProxyLogin is empty.")); + return ResultState::FATAL_ERROR; + } Opts = Opts->Child; // Iterate over the entire login script @@ -274,9 +298,12 @@ bool FTPConn::Login() // Send the command if (WriteMsg(Tag,Msg,"%s",Tmp.c_str()) == false) - return false; + return ResultState::TRANSIENT_ERROR; if (Tag >= 400) - return _error->Error(_("Login script command '%s' failed, server said: %s"),Tmp.c_str(),Msg.c_str()); + { + _error->Error(_("Login script command '%s' failed, server said: %s"), Tmp.c_str(), Msg.c_str()); + return ResultState::FATAL_ERROR; + } } // Enter passive mode @@ -300,11 +327,13 @@ bool FTPConn::Login() // Binary mode if (WriteMsg(Tag,Msg,"TYPE I") == false) - return false; + return ResultState::TRANSIENT_ERROR; if (Tag >= 400) - return _error->Error(_("TYPE failed, server said: %s"),Msg.c_str()); - - return true; + { + _error->Error(_("TYPE failed, server said: %s"), Msg.c_str()); + return ResultState::FATAL_ERROR; + } + return ResultState::SUCCESSFUL; } /*}}}*/ // FTPConn::ReadLine - Read a line from the server /*{{{*/ @@ -1023,15 +1052,22 @@ bool FtpMethod::Fetch(FetchItem *Itm) delete Server; Server = new FTPConn(Get); } - + // Could not connect is a transient error.. - if (Server->Open(this) == false) + switch (Server->Open(this)) { + case ResultState::TRANSIENT_ERROR: Server->Close(); Fail(true); return true; + case ResultState::FATAL_ERROR: + Server->Close(); + Fail(false); + return true; + case ResultState::SUCCESSFUL: + break; } - + // Get the files information Status(_("Query")); unsigned long long Size; diff --git a/methods/ftp.h b/methods/ftp.h index 1859ddce0..52b856637 100644 --- a/methods/ftp.h +++ b/methods/ftp.h @@ -43,7 +43,7 @@ class FTPConn // Private helper functions bool ReadLine(std::string &Text); - bool Login(); + ResultState Login(); bool CreateDataFd(); bool Finalize(); @@ -56,7 +56,7 @@ class FTPConn bool WriteMsg(unsigned int &Ret,std::string &Text,const char *Fmt,...); // Connection control - bool Open(aptMethod *Owner); + ResultState Open(aptMethod *Owner); void Close(); bool GoPasv(); bool ExtGoPasv(); diff --git a/methods/http.cc b/methods/http.cc index b7c9fe349..2d23b1646 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -328,8 +328,8 @@ struct HttpConnectFd : public MethodFd } }; -bool UnwrapHTTPConnect(std::string Host, int Port, URI Proxy, std::unique_ptr &Fd, - unsigned long Timeout, aptAuthConfMethod *Owner) +static ResultState UnwrapHTTPConnect(std::string Host, int Port, URI Proxy, std::unique_ptr &Fd, + unsigned long Timeout, aptAuthConfMethod *Owner) { Owner->Status(_("Connecting to %s (%s)"), "HTTP proxy", URI::SiteOnly(Proxy).c_str()); // The HTTP server expects a hostname with a trailing :port @@ -369,17 +369,29 @@ bool UnwrapHTTPConnect(std::string Host, int Port, URI Proxy, std::unique_ptr 0) { if (WaitFd(Fd->Fd(), true, Timeout) == false) - return _error->Errno("select", "Writing to proxy failed"); + { + _error->Errno("select", "Writing to proxy failed"); + return ResultState::TRANSIENT_ERROR; + } if (Out.Write(Fd) == false) - return _error->Errno("write", "Writing to proxy failed"); + { + _error->Errno("write", "Writing to proxy failed"); + return ResultState::TRANSIENT_ERROR; + } } while (In.ReadSpace() > 0) { if (WaitFd(Fd->Fd(), false, Timeout) == false) - return _error->Errno("select", "Reading from proxy failed"); + { + _error->Errno("select", "Reading from proxy failed"); + return ResultState::TRANSIENT_ERROR; + } if (In.Read(Fd) == false) - return _error->Errno("read", "Reading from proxy failed"); + { + _error->Errno("read", "Reading from proxy failed"); + return ResultState::TRANSIENT_ERROR; + } if (In.WriteTillEl(Headers)) break; @@ -389,7 +401,10 @@ bool UnwrapHTTPConnect(std::string Host, int Port, URI Proxy, std::unique_ptrError("Invalid response from proxy: %s", Headers.c_str()); + { + _error->Error("Invalid response from proxy: %s", Headers.c_str()); + return ResultState::TRANSIENT_ERROR; + } if (In.WriteSpace() > 0) { @@ -400,7 +415,7 @@ bool UnwrapHTTPConnect(std::string Host, int Port, URI Proxy, std::unique_ptrFd() != -1) - return true; - + return ResultState::SUCCESSFUL; + Close(); In.Reset(); Out.Reset(); @@ -476,12 +491,14 @@ bool HttpServerState::Open() auto const DefaultPort = tls ? 443 : 80; if (Proxy.Access == "socks5h") { - if (Connect(Proxy.Host, Proxy.Port, "socks", 1080, ServerFd, TimeOut, Owner) == false) - return false; - - if (UnwrapSocks(ServerName.Host, ServerName.Port == 0 ? DefaultPort : ServerName.Port, - Proxy, ServerFd, Owner->ConfigFindI("TimeOut", 120), Owner) == false) - return false; + auto result = Connect(Proxy.Host, Proxy.Port, "socks", 1080, ServerFd, TimeOut, Owner); + if (result != ResultState::SUCCESSFUL) + return result; + + result = UnwrapSocks(ServerName.Host, ServerName.Port == 0 ? DefaultPort : ServerName.Port, + Proxy, ServerFd, Owner->ConfigFindI("TimeOut", 120), Owner); + if (result != ResultState::SUCCESSFUL) + return result; } else { @@ -495,7 +512,10 @@ bool HttpServerState::Open() Host = ServerName.Host; } else if (Proxy.Access != "http" && Proxy.Access != "https") - return _error->Error("Unsupported proxy configured: %s", URI::SiteOnly(Proxy).c_str()); + { + _error->Error("Unsupported proxy configured: %s", URI::SiteOnly(Proxy).c_str()); + return ResultState::FATAL_ERROR; + } else { if (Proxy.Port != 0) @@ -505,18 +525,27 @@ bool HttpServerState::Open() if (Proxy.Access == "https" && Port == 0) Port = 443; } - if (!Connect(Host, Port, DefaultService, DefaultPort, ServerFd, TimeOut, Owner)) - return false; - if (Host == Proxy.Host && Proxy.Access == "https" && UnwrapTLS(Proxy.Host, ServerFd, TimeOut, Owner) == false) - return false; - if (Host == Proxy.Host && tls && UnwrapHTTPConnect(ServerName.Host, ServerName.Port == 0 ? DefaultPort : ServerName.Port, Proxy, ServerFd, Owner->ConfigFindI("TimeOut", 120), Owner) == false) - return false; + auto result = Connect(Host, Port, DefaultService, DefaultPort, ServerFd, TimeOut, Owner); + if (result != ResultState::SUCCESSFUL) + return result; + if (Host == Proxy.Host && Proxy.Access == "https") + { + result = UnwrapTLS(Proxy.Host, ServerFd, TimeOut, Owner); + if (result != ResultState::SUCCESSFUL) + return result; + } + if (Host == Proxy.Host && tls) + { + result = UnwrapHTTPConnect(ServerName.Host, ServerName.Port == 0 ? DefaultPort : ServerName.Port, Proxy, ServerFd, Owner->ConfigFindI("TimeOut", 120), Owner); + if (result != ResultState::SUCCESSFUL) + return result; + } } - if (tls && UnwrapTLS(ServerName.Host, ServerFd, TimeOut, Owner) == false) - return false; + if (tls) + return UnwrapTLS(ServerName.Host, ServerFd, TimeOut, Owner); - return true; + return ResultState::SUCCESSFUL; } /*}}}*/ // HttpServerState::Close - Close a connection to the server /*{{{*/ @@ -529,7 +558,7 @@ bool HttpServerState::Close() } /*}}}*/ // HttpServerState::RunData - Transfer the data from the socket /*{{{*/ -bool HttpServerState::RunData(RequestState &Req) +ResultState HttpServerState::RunData(RequestState &Req) { Req.State = RequestState::Data; @@ -539,19 +568,18 @@ bool HttpServerState::RunData(RequestState &Req) while (1) { // Grab the block size - bool Last = true; + ResultState Last = ResultState::SUCCESSFUL; string Data; In.Limit(-1); do { if (In.WriteTillEl(Data,true) == true) break; - } - while ((Last = Go(false, Req)) == true); + } while ((Last = Go(false, Req)) == ResultState::SUCCESSFUL); + + if (Last != ResultState::SUCCESSFUL) + return Last; - if (Last == false) - return false; - // See if we are done unsigned long long Len = strtoull(Data.c_str(),0,16); if (Len == 0) @@ -559,39 +587,35 @@ bool HttpServerState::RunData(RequestState &Req) In.Limit(-1); // We have to remove the entity trailer - Last = true; + Last = ResultState::SUCCESSFUL; do { if (In.WriteTillEl(Data,true) == true && Data.length() <= 2) break; - } - while ((Last = Go(false, Req)) == true); - if (Last == false) - return false; - return !_error->PendingError(); + } while ((Last = Go(false, Req)) == ResultState::SUCCESSFUL); + return Last; } - + // Transfer the block In.Limit(Len); - while (Go(true, Req) == true) + while (Go(true, Req) == ResultState::SUCCESSFUL) if (In.IsLimit() == true) break; // Error if (In.IsLimit() == false) - return false; - + return ResultState::TRANSIENT_ERROR; + // The server sends an extra new line before the next block specifier.. In.Limit(-1); - Last = true; + Last = ResultState::SUCCESSFUL; do { if (In.WriteTillEl(Data,true) == true) break; - } - while ((Last = Go(false, Req)) == true); - if (Last == false) - return false; + } while ((Last = Go(false, Req)) == ResultState::SUCCESSFUL); + if (Last != ResultState::SUCCESSFUL) + return Last; } } else @@ -605,34 +629,41 @@ bool HttpServerState::RunData(RequestState &Req) if (Req.MaximumSize != 0 && Req.DownloadSize > Req.MaximumSize) { Owner->SetFailReason("MaximumSizeExceeded"); - return _error->Error(_("File has unexpected size (%llu != %llu). Mirror sync in progress?"), - Req.DownloadSize, Req.MaximumSize); + _error->Error(_("File has unexpected size (%llu != %llu). Mirror sync in progress?"), + Req.DownloadSize, Req.MaximumSize); + return ResultState::FATAL_ERROR; } In.Limit(Req.DownloadSize); } else if (Persistent == false) In.Limit(-1); - + // Just transfer the whole block. - do + while (true) { if (In.IsLimit() == false) - continue; - + { + auto const result = Go(true, Req); + if (result == ResultState::SUCCESSFUL) + continue; + return result; + } + In.Limit(-1); - return !_error->PendingError(); + return _error->PendingError() ? ResultState::FATAL_ERROR : ResultState::SUCCESSFUL; } - while (Go(true, Req) == true); } - return Flush(&Req.File) && !_error->PendingError(); + if (Flush(&Req.File) == false) + return ResultState::TRANSIENT_ERROR; + return ResultState::SUCCESSFUL; } /*}}}*/ -bool HttpServerState::RunDataToDevNull(RequestState &Req) /*{{{*/ +ResultState HttpServerState::RunDataToDevNull(RequestState &Req) /*{{{*/ { // no need to clean up if we discard the connection anyhow if (Persistent == false) - return true; + return ResultState::SUCCESSFUL; Req.File.Open("/dev/null", FileFd::WriteOnly); return RunData(Req); } @@ -642,7 +673,7 @@ bool HttpServerState::ReadHeaderLines(std::string &Data) /*{{{*/ return In.WriteTillEl(Data); } /*}}}*/ -bool HttpServerState::LoadNextResponse(bool const ToFile, RequestState &Req)/*{{{*/ +ResultState HttpServerState::LoadNextResponse(bool const ToFile, RequestState &Req) /*{{{*/ { return Go(ToFile, Req); } @@ -677,7 +708,7 @@ APT_PURE Hashes * HttpServerState::GetHashes() /*{{{*/ } /*}}}*/ // HttpServerState::Die - The server has closed the connection. /*{{{*/ -bool HttpServerState::Die(RequestState &Req) +ResultState HttpServerState::Die(RequestState &Req) { unsigned int LErrno = errno; @@ -685,7 +716,7 @@ bool HttpServerState::Die(RequestState &Req) if (Req.State == RequestState::Data) { if (Req.File.IsOpen() == false) - return true; + return ResultState::SUCCESSFUL; // on GNU/kFreeBSD, apt dies on /dev/null because non-blocking // can't be set if (Req.File.Name() != "/dev/null") @@ -693,11 +724,14 @@ bool HttpServerState::Die(RequestState &Req) while (In.WriteSpace() == true) { if (In.Write(MethodFd::FromFd(Req.File.Fd())) == false) - return _error->Errno("write",_("Error writing to the file")); + { + _error->Errno("write", _("Error writing to the file")); + return ResultState::TRANSIENT_ERROR; + } // Done if (In.IsLimit() == true) - return true; + return ResultState::SUCCESSFUL; } } @@ -707,9 +741,13 @@ bool HttpServerState::Die(RequestState &Req) { Close(); if (LErrno == 0) - return _error->Error(_("Error reading from server. Remote end closed connection")); + { + _error->Error(_("Error reading from server. Remote end closed connection")); + return ResultState::TRANSIENT_ERROR; + } errno = LErrno; - return _error->Errno("read",_("Error reading from server")); + _error->Errno("read", _("Error reading from server")); + return ResultState::TRANSIENT_ERROR; } else { @@ -717,14 +755,14 @@ bool HttpServerState::Die(RequestState &Req) // Nothing left in the buffer if (In.WriteSpace() == false) - return false; + return ResultState::TRANSIENT_ERROR; // We may have got multiple responses back in one packet.. Close(); - return true; + return ResultState::SUCCESSFUL; } - return false; + return ResultState::TRANSIENT_ERROR; } /*}}}*/ // HttpServerState::Flush - Dump the buffer into the file /*{{{*/ @@ -760,12 +798,12 @@ bool HttpServerState::Flush(FileFd * const File) // --------------------------------------------------------------------- /* This runs the select loop over the server FDs, Output file FDs and stdin. */ -bool HttpServerState::Go(bool ToFile, RequestState &Req) +ResultState HttpServerState::Go(bool ToFile, RequestState &Req) { // Server has closed the connection if (ServerFd->Fd() == -1 && (In.WriteSpace() == false || ToFile == false)) - return false; + return ResultState::TRANSIENT_ERROR; // Handle server IO if (ServerFd->HasPending() && In.ReadSpace() == true) @@ -811,8 +849,9 @@ bool HttpServerState::Go(bool ToFile, RequestState &Req) if ((Res = select(MaxFd+1,&rfds,&wfds,0,&tv)) < 0) { if (errno == EINTR) - return true; - return _error->Errno("select",_("Select failed")); + return ResultState::SUCCESSFUL; + _error->Errno("select", _("Select failed")); + return ResultState::TRANSIENT_ERROR; } if (Res == 0) @@ -840,14 +879,18 @@ bool HttpServerState::Go(bool ToFile, RequestState &Req) if (FileFD->Fd() != -1 && FD_ISSET(FileFD->Fd(), &wfds)) { if (In.Write(FileFD) == false) - return _error->Errno("write",_("Error writing to output file")); + { + _error->Errno("write", _("Error writing to output file")); + return ResultState::TRANSIENT_ERROR; + } } if (Req.MaximumSize > 0 && Req.File.IsOpen() && Req.File.Failed() == false && Req.File.Tell() > Req.MaximumSize) { Owner->SetFailReason("MaximumSizeExceeded"); - return _error->Error(_("File has unexpected size (%llu != %llu). Mirror sync in progress?"), - Req.File.Tell(), Req.MaximumSize); + _error->Error(_("File has unexpected size (%llu != %llu). Mirror sync in progress?"), + Req.File.Tell(), Req.MaximumSize); + return ResultState::FATAL_ERROR; } // Handle commands from APT @@ -855,9 +898,9 @@ bool HttpServerState::Go(bool ToFile, RequestState &Req) { if (Owner->Run(true) != -1) exit(100); - } - - return true; + } + + return ResultState::SUCCESSFUL; } /*}}}*/ diff --git a/methods/http.h b/methods/http.h index 6d44fbdd4..84cc0b2b1 100644 --- a/methods/http.h +++ b/methods/http.h @@ -93,8 +93,6 @@ class CircleBuf ~CircleBuf(); }; -bool UnwrapHTTPConnect(std::string To, int Port, URI Proxy, std::unique_ptr &Fd, unsigned long Timeout, aptAuthConfMethod *Owner); - struct HttpServerState: public ServerState { // This is the connection itself. Output is data FROM the server @@ -104,23 +102,23 @@ struct HttpServerState: public ServerState protected: virtual bool ReadHeaderLines(std::string &Data) APT_OVERRIDE; - virtual bool LoadNextResponse(bool const ToFile, RequestState &Req) APT_OVERRIDE; + virtual ResultState LoadNextResponse(bool const ToFile, RequestState &Req) APT_OVERRIDE; virtual bool WriteResponse(std::string const &Data) APT_OVERRIDE; public: virtual void Reset() APT_OVERRIDE; - virtual bool RunData(RequestState &Req) APT_OVERRIDE; - virtual bool RunDataToDevNull(RequestState &Req) APT_OVERRIDE; + virtual ResultState RunData(RequestState &Req) APT_OVERRIDE; + virtual ResultState RunDataToDevNull(RequestState &Req) APT_OVERRIDE; - virtual bool Open() APT_OVERRIDE; + virtual ResultState Open() APT_OVERRIDE; virtual bool IsOpen() APT_OVERRIDE; virtual bool Close() APT_OVERRIDE; virtual bool InitHashes(HashStringList const &ExpectedHashes) APT_OVERRIDE; virtual Hashes * GetHashes() APT_OVERRIDE; - virtual bool Die(RequestState &Req) APT_OVERRIDE; + virtual ResultState Die(RequestState &Req) APT_OVERRIDE; virtual bool Flush(FileFd * const File) APT_OVERRIDE; - virtual bool Go(bool ToFile, RequestState &Req) APT_OVERRIDE; + virtual ResultState Go(bool ToFile, RequestState &Req) APT_OVERRIDE; HttpServerState(URI Srv, HttpMethod *Owner); virtual ~HttpServerState() {Close();}; -- cgit v1.2.3 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 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 From 9f572c0a6d13cc983a4f8880a3dee3a8e46604bb Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 27 Oct 2017 18:38:47 +0200 Subject: give the methods more metadata about the files to acquire We have quite a bit of metadata available for the files we acquire, but the methods weren't told about it and got just the URI. That is indeed fine for most, but to avoid methods trying to parse the metadata out of the provided URIs (and fail horribly in edgecases) we can just as well be nice and tell them stuff directly. --- apt-pkg/acquire-item.cc | 40 +++++++++++++++++++++++++++++++++++++--- apt-pkg/acquire-item.h | 3 +++ apt-pkg/deb/debmetaindex.cc | 18 +++++++++--------- 3 files changed, 49 insertions(+), 12 deletions(-) diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 8c11337ac..0e73b3b8c 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -684,6 +684,7 @@ class pkgAcquire::Item::Private { public: std::vector PastRedirections; + std::unordered_map CustomFields; unsigned int Retries; Private() : Retries(_config->FindI("Acquire::Retries", 0)) @@ -709,7 +710,17 @@ pkgAcquire::Item::~Item() /*}}}*/ std::string pkgAcquire::Item::Custom600Headers() const /*{{{*/ { - return std::string(); + std::ostringstream header; + for (auto const &f : d->CustomFields) + if (f.second.empty() == false) + header << '\n' + << f.first << ": " << f.second; + return header.str(); +} + /*}}}*/ +std::unordered_map &pkgAcquire::Item::ModifyCustomFields() /*{{{*/ +{ + return d->CustomFields; } /*}}}*/ unsigned int &pkgAcquire::Item::ModifyRetries() /*{{{*/ @@ -1051,6 +1062,15 @@ pkgAcqTransactionItem::pkgAcqTransactionItem(pkgAcquire * const Owner, /*{{{*/ { if (TransactionManager != this) TransactionManager->Add(this); + ModifyCustomFields() = { + {"Target-Site", Target.Option(IndexTarget::SITE)}, + {"Target-Repo-URI", Target.Option(IndexTarget::REPO_URI)}, + {"Target-Base-URI", Target.Option(IndexTarget::BASE_URI)}, + {"Target-Component", Target.Option(IndexTarget::COMPONENT)}, + {"Target-Release", Target.Option(IndexTarget::RELEASE)}, + {"Target-Architecture", Target.Option(IndexTarget::ARCHITECTURE)}, + {"Target-Language", Target.Option(IndexTarget::LANGUAGE)}, + }; } /*}}}*/ pkgAcqTransactionItem::~pkgAcqTransactionItem() /*{{{*/ @@ -1228,7 +1248,8 @@ bool pkgAcqMetaBase::CheckStopAuthentication(pkgAcquire::Item * const I, const s // --------------------------------------------------------------------- string pkgAcqMetaBase::Custom600Headers() const { - std::string Header = "\nIndex-File: true"; + std::string Header = pkgAcqTransactionItem::Custom600Headers(); + Header.append("\nIndex-File: true"); std::string MaximumSize; strprintf(MaximumSize, "\nMaximum-Size: %i", _config->FindI("Acquire::MaxReleaseFileSize", 10*1000*1000)); @@ -3219,7 +3240,6 @@ void pkgAcqIndex::StageDecompressDone() /*}}}*/ pkgAcqIndex::~pkgAcqIndex() {} - // AcqArchive::AcqArchive - Constructor /*{{{*/ // --------------------------------------------------------------------- /* This just sets up the initial fetch environment and queues the first @@ -3346,6 +3366,20 @@ bool pkgAcqArchive::QueueNext() Desc.Owner = this; Desc.ShortDesc = Version.ParentPkg().FullName(true); + auto fields = ModifyCustomFields(); + if (PkgF->Architecture != 0) + fields.emplace("Target-Architecture", PkgF.Architecture()); + if (PkgF->Component != 0) + fields.emplace("Target-Component", PkgF.Component()); + auto const RelF = PkgF.ReleaseFile(); + if (RelF.end() == false) + { + if (RelF->Codename != 0) + fields.emplace("Target-Codename", RelF.Codename()); + if (RelF->Archive != 0) + fields.emplace("Target-Suite", RelF.Archive()); + } + // See if we already have the file. (Legacy filenames) FileSize = Version->Size; string FinalFile = _config->FindDir("Dir::Cache::Archives") + flNotDir(PkgFile); diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index 7705f3ccb..7f5f75195 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -27,6 +27,7 @@ #include #include +#include #include #ifndef APT_8_CLEANER_HEADERS @@ -241,6 +242,8 @@ class pkgAcquire::Item : public WeakPointable /*{{{*/ 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(); + // this is more a hack than a proper external interface, hence hidden + APT_HIDDEN std::unordered_map &ModifyCustomFields(); /** \brief A "descriptive" URI-like string. * diff --git a/apt-pkg/deb/debmetaindex.cc b/apt-pkg/deb/debmetaindex.cc index ad27e2dcd..2688052a4 100644 --- a/apt-pkg/deb/debmetaindex.cc +++ b/apt-pkg/deb/debmetaindex.cc @@ -118,8 +118,6 @@ static void GetIndexTargetsFor(char const * const Type, std::string const &URI, { bool const flatArchive = (Dist[Dist.length() - 1] == '/'); std::string const baseURI = constructMetaIndexURI(URI, Dist, ""); - std::string const Release = (Dist == "/") ? "" : Dist; - std::string const Site = ::URI::ArchiveOnly(URI); std::string DefCompressionTypes; { @@ -208,8 +206,7 @@ static void GetIndexTargetsFor(char const * const Type, std::string const &URI, constexpr static auto BreakPoint = "$(NATIVE_ARCHITECTURE)"; // available in templates std::map Options; - Options.insert(std::make_pair("SITE", Site)); - Options.insert(std::make_pair("RELEASE", Release)); + Options.insert(ReleaseOptions.begin(), ReleaseOptions.end()); if (tplMetaKey.find("$(COMPONENT)") != std::string::npos) Options.emplace("COMPONENT", E->Name); if (tplMetaKey.find("$(LANGUAGE)") != std::string::npos) @@ -290,7 +287,6 @@ static void GetIndexTargetsFor(char const * const Type, std::string const &URI, } // not available in templates, but in the indextarget - Options.insert(ReleaseOptions.begin(), ReleaseOptions.end()); Options.insert(std::make_pair("IDENTIFIER", Identifier)); Options.insert(std::make_pair("TARGET_OF", Type)); Options.insert(std::make_pair("CREATED_BY", *T)); @@ -1070,7 +1066,7 @@ class APT_HIDDEN debSLTypeDebian : public pkgSourceList::Type /*{{{*/ } for (auto&& key: KeysA) { - if (key == "BASE_URI" || key == "REPO_URI") + if (key == "BASE_URI" || key == "REPO_URI" || key == "SITE" || key == "RELEASE") continue; auto const a = OptionsA.find(key); auto const b = OptionsB.find(key); @@ -1083,9 +1079,11 @@ class APT_HIDDEN debSLTypeDebian : public pkgSourceList::Type /*{{{*/ static debReleaseIndex * GetDebReleaseIndexBy(std::vector &List, std::string const &URI, std::string const &Dist, std::map const &Options) { - std::map ReleaseOptions = {{ - { "BASE_URI", constructMetaIndexURI(URI, Dist, "") }, - { "REPO_URI", URI }, + std::map ReleaseOptions{{ + {"BASE_URI", constructMetaIndexURI(URI, Dist, "")}, + {"REPO_URI", URI}, + {"SITE", ::URI::ArchiveOnly(URI)}, + {"RELEASE", (Dist == "/") ? "" : Dist}, }}; if (GetBoolOption(Options, "allow-insecure", _config->FindB("Acquire::AllowInsecureRepositories"))) ReleaseOptions.emplace("ALLOW_INSECURE", "true"); @@ -1134,6 +1132,8 @@ class APT_HIDDEN debSLTypeDebian : public pkgSourceList::Type /*{{{*/ bool const &IsSrc, std::map const &Options) const { auto const Deb = GetDebReleaseIndexBy(List, URI, Dist, Options); + if (Deb == nullptr) + return false; bool const UsePDiffs = GetBoolOption(Options, "pdiffs", _config->FindB("Acquire::PDiffs", true)); -- cgit v1.2.3 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(-) 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