Merge branch 'pu/http-debug' into 'master'

Add better acquire debugging support

See merge request apt-team/apt!130

4 files changed, 58 insertions, 66 deletions

diff --git a/apt-pkg/acquire-method.cc b/apt-pkg/acquire-method.cc
index 9656caf14..486098c77 100644
--- a/apt-pkg/acquire-method.cc
+++ b/apt-pkg/acquire-method.cc
@@ -139,27 +139,30 @@ void pkgAcqMethod::SendMessage(std::string const &header, std::unordered_map<std
 /* */
 void pkgAcqMethod::Fail(bool Transient)
 {
-   string Err = "Undetermined Error";
-   if (_error->empty() == false)
+
+   Fail("", Transient);
+}
+									/*}}}*/
+// AcqMethod::Fail - A fetch has failed					/*{{{*/
+void pkgAcqMethod::Fail(string Err, bool Transient)
+{
+
+   if (not _error->empty())
    {
-      Err.clear();
-      while (_error->empty() == false)
+      while (not _error->empty())
       {
 	 std::string msg;
 	 if (_error->PopMessage(msg))
 	 {
-	    if (Err.empty() == false)
+	    if (not Err.empty())
 	       Err.append("\n");
 	    Err.append(msg);
 	 }
       }
    }
-   Fail(Err, Transient);
-}
-									/*}}}*/
-// AcqMethod::Fail - A fetch has failed					/*{{{*/
-void pkgAcqMethod::Fail(string Err,bool Transient)
-{
+   if (Err.empty())
+      Err = "Undetermined Error";
+
    // Strip out junk from the error messages
    std::transform(Err.begin(), Err.end(), Err.begin(), [](char const c) {
       if (c == '\r' || c == '\n')
diff --git a/methods/basehttp.h b/methods/basehttp.h
index 5fdff69e0..4a83f319c 100644
--- a/methods/basehttp.h
+++ b/methods/basehttp.h
@@ -107,7 +107,7 @@ struct ServerState
    virtual bool Close() = 0;
    virtual bool InitHashes(HashStringList const &ExpectedHashes) = 0;
    virtual ResultState Die(RequestState &Req) = 0;
-   virtual bool Flush(FileFd * const File) = 0;
+   virtual bool Flush(FileFd *const File, bool MustComplete = false) = 0;
    virtual ResultState Go(bool ToFile, RequestState &Req) = 0;
    virtual Hashes * GetHashes() = 0;
 
diff --git a/methods/http.cc b/methods/http.cc
index 77348d760..0ef695166 100644
--- a/methods/http.cc
+++ b/methods/http.cc
@@ -702,43 +702,30 @@ ResultState HttpServerState::Die(RequestState &Req)
 
    Close();
 
-   // Dump the buffer to the file
-   if (Req.State == RequestState::Data)
+   switch (Req.State)
    {
-      // on GNU/kFreeBSD, apt dies on /dev/null because non-blocking
-      // can't be set
-      if (Req.File.Name() != "/dev/null")
-	 SetNonBlock(Req.File.Fd(),false);
-      if (In.WriteSpace()) {
-	 _error->Error(_("Data left in buffer"));
-	 return ResultState::TRANSIENT_ERROR;
-      }
+   case RequestState::Data:
+      // We have read all data we could, or the connection is not persistent
+      if (In.IsLimit() == true || Persistent == false)
+	 return ResultState::SUCCESSFUL;
+      break;
+   case RequestState::Header:
+      In.Limit(-1);
+      // We have read some headers, but we might also have read the content
+      // and an EOF and hence reached this point. This is fine.
+      if (In.WriteSpace())
+	 return ResultState::SUCCESSFUL;
+      break;
    }
 
-   // See if this is because the server finished the data stream
-   if (In.IsLimit() == false && Req.State != RequestState::Header &&
-       Persistent == true)
+   // We have reached an actual error, tell the user about it.
+   if (LErrno == 0)
    {
-      if (LErrno == 0)
-      {
-	 _error->Error(_("Error reading from server. Remote end closed connection"));
-	 return ResultState::TRANSIENT_ERROR;
-      }
-      errno = LErrno;
-      _error->Errno("read", _("Error reading from server"));
+      _error->Error(_("Error reading from server. Remote end closed connection"));
       return ResultState::TRANSIENT_ERROR;
    }
-   else
-   {
-      In.Limit(-1);
-
-      // Nothing left in the buffer
-      if (In.WriteSpace() == false)
-	 return ResultState::TRANSIENT_ERROR;
-
-      // We may have got multiple responses back in one packet..
-      return ResultState::SUCCESSFUL;
-   }
+   errno = LErrno;
+   _error->Errno("read", _("Error reading from server"));
 
    return ResultState::TRANSIENT_ERROR;
 }
@@ -747,14 +734,10 @@ ResultState HttpServerState::Die(RequestState &Req)
 // ---------------------------------------------------------------------
 /* This takes the current input buffer from the Server FD and writes it
    into the file */
-bool HttpServerState::Flush(FileFd * const File)
+bool HttpServerState::Flush(FileFd *const File, bool MustComplete)
 {
    if (File != nullptr)
    {
-      // on GNU/kFreeBSD, apt dies on /dev/null because non-blocking
-      // can't be set
-      if (File->Name() != "/dev/null")
-	 SetNonBlock(File->Fd(),false);
       if (In.WriteSpace() == false)
 	 return true;
       
@@ -766,7 +749,7 @@ bool HttpServerState::Flush(FileFd * const File)
 	    return true;
       }
 
-      if (In.IsLimit() == true || Persistent == false)
+      if (In.IsLimit() == true || Persistent == false || not MustComplete)
 	 return true;
    }
    return false;
@@ -800,20 +783,23 @@ ResultState HttpServerState::Go(bool ToFile, RequestState &Req)
    if (In.ReadSpace() == true && ServerFd->Fd() != -1)
       FD_SET(ServerFd->Fd(), &rfds);
 
-   // Add the file
-   auto FileFD = MethodFd::FromFd(-1);
-   if (Req.File.IsOpen())
-      FileFD = MethodFd::FromFd(Req.File.Fd());
-
-   if (In.WriteSpace() == true && ToFile == true && FileFD->Fd() != -1)
-      FD_SET(FileFD->Fd(), &wfds);
+   // Add the file. Note that we need to add the file to the select and
+   // then write before we read from the server so we do not have content
+   // left to write if the server closes the connection when we read from it.
+   //
+   // An alternative would be to just flush the file in those circumstances
+   // and then return. Because otherwise we might end up blocking indefinitely
+   // in the select() call if we were to continue but all that was left to do
+   // was write to the local file.
+   if (In.WriteSpace() == true && ToFile == true && Req.File.IsOpen())
+      FD_SET(Req.File.Fd(), &wfds);
 
    // Add stdin
    if (Owner->ConfigFindB("DependOnSTDIN", true) == true)
       FD_SET(STDIN_FILENO,&rfds);
 	  
    // Figure out the max fd
-   int MaxFd = FileFD->Fd();
+   int MaxFd = Req.File.Fd();
    if (MaxFd < ServerFd->Fd())
       MaxFd = ServerFd->Fd();
 
@@ -835,7 +821,15 @@ ResultState HttpServerState::Go(bool ToFile, RequestState &Req)
       _error->Error(_("Connection timed out"));
       return ResultState::TRANSIENT_ERROR;
    }
-   
+
+   // Flush any data before talking to the server, in case the server
+   // closed the connection, we want to be done writing.
+   if (Req.File.IsOpen() && FD_ISSET(Req.File.Fd(), &wfds))
+   {
+      if (not Flush(&Req.File, false))
+	 return ResultState::TRANSIENT_ERROR;
+   }
+
    // Handle server IO
    if (ServerPending || (ServerFd->Fd() != -1 && FD_ISSET(ServerFd->Fd(), &rfds)))
    {
@@ -845,14 +839,10 @@ ResultState HttpServerState::Go(bool ToFile, RequestState &Req)
    }
 
    // Send data to the file
-   if (FileFD->Fd() != -1 && ((In.WriteSpace() == true && ToFile == true) ||
-			      FD_ISSET(FileFD->Fd(), &wfds)))
+   if (In.WriteSpace() == true && ToFile == true && Req.File.IsOpen())
    {
-      if (In.Write(FileFD) == false)
-      {
-	 _error->Errno("write", _("Error writing to output file"));
+      if (not Flush(&Req.File, false))
 	 return ResultState::TRANSIENT_ERROR;
-      }
    }
 
    if (ServerFd->Fd() != -1 && FD_ISSET(ServerFd->Fd(), &wfds))
@@ -1029,7 +1019,6 @@ BaseHttpMethod::DealWithHeadersResult HttpMethod::DealWithHeaders(FetchResult &R
    if (Req.StartPos > 0)
       Res.ResumePoint = Req.StartPos;
 
-   SetNonBlock(Req.File.Fd(),true);
    return FILE_IS_OPEN;
 }
 									/*}}}*/
diff --git a/methods/http.h b/methods/http.h
index 5668f0b87..cae579afe 100644
--- a/methods/http.h
+++ b/methods/http.h
@@ -114,7 +114,7 @@ struct HttpServerState: public ServerState
    virtual bool InitHashes(HashStringList const &ExpectedHashes) APT_OVERRIDE;
    virtual Hashes * GetHashes() APT_OVERRIDE;
    virtual ResultState Die(RequestState &Req) APT_OVERRIDE;
-   virtual bool Flush(FileFd * const File) APT_OVERRIDE;
+   virtual bool Flush(FileFd *const File, bool MustComplete = true) APT_OVERRIDE;
    virtual ResultState Go(bool ToFile, RequestState &Req) APT_OVERRIDE;
 
    HttpServerState(URI Srv, HttpMethod *Owner);