From 2be10edfb96f8323f0ffc6bf70bfca01d41bd265 Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Tue, 23 Nov 2021 13:29:24 -0500 Subject: [PATCH] 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 --- src/kits/package/DownloadFileRequest.cpp | 19 +++++---- src/kits/package/FetchUtils.cpp | 4 +- src/kits/package/FetchUtils.h | 4 +- src/kits/package/manager/PackageManager.cpp | 47 ++++++++++++--------- 4 files changed, 43 insertions(+), 31 deletions(-) diff --git a/src/kits/package/DownloadFileRequest.cpp b/src/kits/package/DownloadFileRequest.cpp index 3ca08200a3..8251bd76c1 100644 --- a/src/kits/package/DownloadFileRequest.cpp +++ b/src/kits/package/DownloadFileRequest.cpp @@ -12,6 +12,7 @@ #include #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 - FetchFileJob* fetchJob = new (std::nothrow) FetchFileJob(fContext, - BString("Downloading ") << fFileURL, fFileURL, fTargetEntry); - if (fetchJob == NULL) - return B_NO_MEMORY; + if (!FetchUtils::IsDownloadCompleted(BNode(&fTargetEntry))) { + // create the download job + FetchFileJob* fetchJob = new (std::nothrow) FetchFileJob(fContext, + BString("Downloading ") << fFileURL, fFileURL, fTargetEntry); + if (fetchJob == NULL) + return B_NO_MEMORY; - if ((error = QueueJob(fetchJob)) != B_OK) { - delete fetchJob; - return error; + if ((error = QueueJob(fetchJob)) != B_OK) { + delete fetchJob; + return error; + } } // create the checksum validation job diff --git a/src/kits/package/FetchUtils.cpp b/src/kits/package/FetchUtils.cpp index 046d7f8d87..104f01c4bb 100644 --- a/src/kits/package/FetchUtils.cpp +++ b/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) diff --git a/src/kits/package/FetchUtils.h b/src/kits/package/FetchUtils.h index 9abb1682c9..1f3ea29572 100644 --- a/src/kits/package/FetchUtils.h +++ b/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); diff --git a/src/kits/package/manager/PackageManager.cpp b/src/kits/package/manager/PackageManager.cpp index bb2252f46a..73ae1a423a 100644 --- a/src/kits/package/manager/PackageManager.cpp +++ b/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(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( - path.Path()); + reusingDownload = true; 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 - // file already exists) - BString url = remoteRepository->Config().PackagesURL(); - url << '/' << fileName; + // download the package (this will resume the download if the + // file already exists) + BString url = remoteRepository->Config().PackagesURL(); + url << '/' << fileName; - status_t error = DownloadPackage(url, entry, - package->Info().Checksum()); - if (error != B_OK) { - if (error == B_BAD_DATA) { - // B_BAD_DATA is returned when there is a checksum - // mismatch. Make sure this download is not re-used. - entry.Remove(); + status_t error; +retryDownload: + error = DownloadPackage(url, entry, + package->Info().Checksum()); + if (error != B_OK) { + if (error == B_BAD_DATA || error == ERANGE) { + // 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