Package Kit: Improvements to partial download handling.

* Move IsDownloadComplete call into DownloadFileRequest; this way
   we will always revalidate checksums even if the file is fully
   downloaded instead of skipping that.

 * Treat ERANGE as a "bad data" error in PackageManager (it usually means
   we have a size mismatch between the local and the server's file)

 * If we fail checksum validation or get ERANGE, and we reused a download,
   immediately try again without reusing the download. This fixes most
   problems that previously required you to delete old transaction
   directories.

Change-Id: Ia3079655691b871e0b206e366b59fca0f832c63d
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4730
Reviewed-by: waddlesplash <[email protected]>

src/kits/package/DownloadFileRequest.cpp

@@ -12,6 +12,7 @@
 #include <package/ValidateChecksumJob.h>
 
 #include "FetchFileJob.h"
+#include "FetchUtils.h"
 
 
 namespace BPackageKit {
@@ -49,15 +50,17 @@ DownloadFileRequest::CreateInitialJobs()
 	if (error != B_OK)
 		return B_NO_INIT;
 
-	// create the download job
+	if (!FetchUtils::IsDownloadCompleted(BNode(&fTargetEntry))) {
-	FetchFileJob* fetchJob = new (std::nothrow) FetchFileJob(fContext,
+		// create the download job
-		BString("Downloading ") << fFileURL, fFileURL, fTargetEntry);
+		FetchFileJob* fetchJob = new (std::nothrow) FetchFileJob(fContext,
-	if (fetchJob == NULL)
+			BString("Downloading ") << fFileURL, fFileURL, fTargetEntry);
-		return B_NO_MEMORY;
+		if (fetchJob == NULL)
+			return B_NO_MEMORY;
 
-	if ((error = QueueJob(fetchJob)) != B_OK) {
+		if ((error = QueueJob(fetchJob)) != B_OK) {
-		delete fetchJob;
+			delete fetchJob;
-		return error;
+			return error;
+		}
 	}
 
 	// create the checksum validation job

src/kits/package/FetchUtils.cpp

@@ -34,7 +34,7 @@ FetchUtils::IsDownloadCompleted(const char* path)
 
 
 /*static*/ bool
-FetchUtils::IsDownloadCompleted(BNode& node)
+FetchUtils::IsDownloadCompleted(const BNode& node)
 {
     bool isComplete;
     status_t status = _GetAttribute(node, DL_COMPLETE_ATTR,
@@ -84,7 +84,7 @@ FetchUtils::_SetAttribute(BNode& node, const char* attrName,
 
 
 status_t
-FetchUtils::_GetAttribute(BNode& node, const char* attrName,
+FetchUtils::_GetAttribute(const BNode& node, const char* attrName,
     type_code type, void* data, size_t size)
 {
 	if (node.InitCheck() != B_OK)

src/kits/package/FetchUtils.h

@@ -17,7 +17,7 @@ namespace BPrivate {
 class FetchUtils {
 public:
 	static	bool				IsDownloadCompleted(const char* path);
-	static	bool				IsDownloadCompleted(BNode& node);
+	static	bool				IsDownloadCompleted(const BNode& node);
 
 	static	status_t			MarkDownloadComplete(const char* path);
 	static	status_t			MarkDownloadComplete(BNode& node);
@@ -29,7 +29,7 @@ private:
 									const char* attrName,
 									type_code type, const void* data,
 									size_t size);
-	static	status_t			_GetAttribute(BNode& node,
+	static	status_t			_GetAttribute(const BNode& node,
 									const char* attrName,
 									type_code type, void* data,
 									size_t size);

src/kits/package/manager/PackageManager.cpp

@@ -34,6 +34,7 @@
 
 #include "FetchFileJob.h"
 #include "FetchUtils.h"
+using BPackageKit::BPrivate::FetchUtils;
 #include "PackageManagerUtils.h"
 
 #undef B_TRANSLATION_CONTEXT
@@ -41,7 +42,6 @@
 
 
 using BPackageKit::BPrivate::FetchFileJob;
-using BPackageKit::BPrivate::FetchUtils;
 using BPackageKit::BPrivate::ValidateChecksumJob;
 
 
@@ -564,7 +564,7 @@ BPackageManager::_PreparePackageChanges(
 		RemoteRepository* remoteRepository
 			= dynamic_cast<RemoteRepository*>(package->Repository());
 		if (remoteRepository != NULL) {
-			bool alreadyDownloaded = false;
+			bool reusingDownload = false;
 
 			// Check for matching files in already existing transaction
 			// directories
@@ -593,33 +593,42 @@ BPackageManager::_PreparePackageChanges(
 					path.Append(fileName);
 					if (bestFile != NULL && BCopyEngine().CopyEntry(bestFile,
 						path.Path()) == B_OK) {
-						alreadyDownloaded = FetchUtils::IsDownloadCompleted(
+						reusingDownload = true;
-							path.Path());
 						printf("Re-using download '%s' from previous "
 							"transaction%s\n", bestFile,
-							alreadyDownloaded ? "" : " (partial)");
+							FetchUtils::IsDownloadCompleted(
+								path.Path()) ? "" : " (partial)");
 					}
 					globfree(&globbuf);
 				}
 			}
 
-			if (!alreadyDownloaded) {
+			// download the package (this will resume the download if the
-				// download the package (this will resume the download if the
+			// file already exists)
-				// file already exists)
+			BString url = remoteRepository->Config().PackagesURL();
-				BString url = remoteRepository->Config().PackagesURL();
+			url << '/' << fileName;
-				url << '/' << fileName;
 
-				status_t error = DownloadPackage(url, entry,
+			status_t error;
-					package->Info().Checksum());
+retryDownload:
-				if (error != B_OK) {
+			error = DownloadPackage(url, entry,
-					if (error == B_BAD_DATA) {
+				package->Info().Checksum());
-						// B_BAD_DATA is returned when there is a checksum
+			if (error != B_OK) {
-						// mismatch. Make sure this download is not re-used.
+				if (error == B_BAD_DATA || error == ERANGE) {
-						entry.Remove();
+					// B_BAD_DATA is returned when there is a checksum
+					// mismatch. Make sure this download is not re-used.
+					entry.Remove();
+
+					if (reusingDownload) {
+						// Maybe the download we reused had some problem.
+						// Try again, this time without reusing the download.
+						printf("\nPrevious download '%s' was invalid. Redownloading.\n",
+							path.Path());
+						reusingDownload = false;
+						goto retryDownload;
 					}
-					DIE(error, "Failed to download package %s",
-						package->Info().Name().String());
 				}
+				DIE(error, "Failed to download package %s",
+					package->Info().Name().String());
 			}
 		} else if (package->Repository() != &installationRepository) {
 			// clone the existing package