From ce3e1d1e7afd2c7b2393f124a10e27d8f82342fd Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sat, 26 Oct 2024 15:48:26 +0200 Subject: [PATCH] libstore: remove FileTransferRequests::data use separate upload and download methods instead. Change-Id: I5baa2177c8ddd70268c75ff074e361b2f17dddbd --- src/libfetchers/tarball.cc | 2 +- src/libstore/filetransfer.cc | 54 +++++++++++++++++-------- src/libstore/filetransfer.hh | 15 ++++--- src/libstore/http-binary-cache-store.cc | 8 ++-- src/nix/upgrade-nix.cc | 2 +- tests/unit/libstore/filetransfer.cc | 4 +- 6 files changed, 56 insertions(+), 29 deletions(-) diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index 11b205383..69ed53f79 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -46,7 +46,7 @@ DownloadFileResult downloadFile( request.expectedETag = getStrAttr(cached->infoAttrs, "etag"); FileTransferResult res; try { - res = getFileTransfer()->enqueueFileTransfer(request).get(); + res = getFileTransfer()->enqueueDownload(request).get(); } catch (FileTransferError & e) { if (cached) { warn("%s; using cached version", e.msg()); diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 95254a78a..394834ad6 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -46,6 +46,7 @@ struct curlFileTransfer : public FileTransfer FileTransferRequest request; FileTransferResult result; Activity act; + std::optional uploadData; bool done = false; // whether either the success or failure function has been called std::packaged_task callback; std::function dataCallback; @@ -81,18 +82,21 @@ struct curlFileTransfer : public FileTransfer std::string verb() const { - return request.data ? "upload" : "download"; + return uploadData ? "upload" : "download"; } TransferItem(curlFileTransfer & fileTransfer, const FileTransferRequest & request, std::invocable auto callback, - std::function dataCallback) + std::function dataCallback, + std::optional uploadData + ) : fileTransfer(fileTransfer) , request(request) , act(*logger, lvlTalkative, actFileTransfer, - fmt(request.data ? "uploading '%s'" : "downloading '%s'", request.uri), + fmt(uploadData ? "uploading '%s'" : "downloading '%s'", request.uri), {request.uri}, request.parentAct) + , uploadData(std::move(uploadData)) , callback([cb{std::move(callback)}] (std::exception_ptr ex, FileTransferResult r) { cb(ex); return r; @@ -236,14 +240,14 @@ struct curlFileTransfer : public FileTransfer size_t readOffset = 0; size_t readCallback(char *buffer, size_t size, size_t nitems) { - if (readOffset == request.data->length()) + if (readOffset == uploadData->length()) return 0; - auto count = std::min(size * nitems, request.data->length() - readOffset); + auto count = std::min(size * nitems, uploadData->length() - readOffset); assert(count); // Lint: this is turning a string into a byte array to hand to // curl, which is fine. // NOLINTNEXTLINE(bugprone-not-null-terminated-result) - memcpy(buffer, request.data->data() + readOffset, count); + memcpy(buffer, uploadData->data() + readOffset, count); readOffset += count; return count; } @@ -295,11 +299,11 @@ struct curlFileTransfer : public FileTransfer if (request.head) curl_easy_setopt(req, CURLOPT_NOBODY, 1); - if (request.data) { + if (uploadData) { curl_easy_setopt(req, CURLOPT_UPLOAD, 1L); curl_easy_setopt(req, CURLOPT_READFUNCTION, readCallbackWrapper); curl_easy_setopt(req, CURLOPT_READDATA, this); - curl_easy_setopt(req, CURLOPT_INFILESIZE_LARGE, (curl_off_t) request.data->length()); + curl_easy_setopt(req, CURLOPT_INFILESIZE_LARGE, (curl_off_t) uploadData->length()); } if (request.verifyTLS) { @@ -620,7 +624,7 @@ struct curlFileTransfer : public FileTransfer std::shared_ptr enqueueItem(std::shared_ptr item) { - if (item->request.data + if (item->uploadData && !item->request.uri.starts_with("http://") && !item->request.uri.starts_with("https://")) throw nix::Error("uploading to '%s' is not supported", item->request.uri); @@ -651,7 +655,19 @@ struct curlFileTransfer : public FileTransfer } #endif - std::future enqueueFileTransfer(const FileTransferRequest & request) override + std::future enqueueDownload(const FileTransferRequest & request) override + { + return enqueueFileTransfer(request, std::nullopt); + } + + std::future + enqueueUpload(const FileTransferRequest & request, std::string data) override + { + return enqueueFileTransfer(request, std::move(data)); + } + + std::future + enqueueFileTransfer(const FileTransferRequest & request, std::optional data) { return enqueueFileTransfer( request, @@ -660,13 +676,16 @@ struct curlFileTransfer : public FileTransfer std::rethrow_exception(ex); } }, - {} + {}, + std::move(data) ); } std::future enqueueFileTransfer(const FileTransferRequest & request, std::invocable auto callback, - std::function dataCallback) + std::function dataCallback, + std::optional data + ) { /* Ugly hack to support s3:// URIs. */ if (request.uri.starts_with("s3://")) { @@ -695,9 +714,11 @@ struct curlFileTransfer : public FileTransfer }); } - return enqueueItem(std::make_shared( - *this, request, std::move(callback), std::move(dataCallback) - )) + return enqueueItem( + std::make_shared( + *this, request, std::move(callback), std::move(dataCallback), std::move(data) + ) + ) ->callback.get_future(); } @@ -747,7 +768,8 @@ struct curlFileTransfer : public FileTransfer thread. */ state->data.append(data); state->avail.notify_one(); - } + }, + std::nullopt ); struct InnerSource : Source diff --git a/src/libstore/filetransfer.hh b/src/libstore/filetransfer.hh index 4865fb59b..3d277c761 100644 --- a/src/libstore/filetransfer.hh +++ b/src/libstore/filetransfer.hh @@ -62,7 +62,6 @@ struct FileTransferRequest size_t tries = fileTransferSettings.tries; unsigned int baseRetryTimeMs = 250; ActivityId parentAct; - std::optional data; FileTransferRequest(std::string_view uri) : uri(uri), parentAct(getCurActivity()) { } @@ -88,11 +87,17 @@ struct FileTransfer virtual ~FileTransfer() { } /** - * Enqueue a data transfer request, returning a future to the result of - * the download. The future may throw a FileTransferError - * exception. + * Enqueues a download request, returning a future for the result of + * the download. The future may throw a FileTransferError exception. */ - virtual std::future enqueueFileTransfer(const FileTransferRequest & request) = 0; + virtual std::future enqueueDownload(const FileTransferRequest & request) = 0; + + /** + * Enqueue an upload request, returning a future for the result of + * the upload. The future may throw a FileTransferError exception. + */ + virtual std::future + enqueueUpload(const FileTransferRequest & request, std::string data) = 0; /** * Download a file, returning its contents through a source. Will not return diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index 36049b325..f54e799c9 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -116,7 +116,7 @@ protected: try { FileTransferRequest request{makeURI(path)}; request.head = true; - getFileTransfer()->enqueueFileTransfer(request).get(); + getFileTransfer()->enqueueDownload(request).get(); return true; } catch (FileTransferError & e) { /* S3 buckets return 403 if a file doesn't exist and the @@ -133,10 +133,10 @@ protected: const std::string & mimeType) override { FileTransferRequest req{makeURI(path)}; - req.data = StreamToSourceAdapter(istream).drain(); + auto data = StreamToSourceAdapter(istream).drain(); req.headers = {{"Content-Type", mimeType}}; try { - getFileTransfer()->enqueueFileTransfer(req).get(); + getFileTransfer()->enqueueUpload(req, std::move(data)).get(); } catch (FileTransferError & e) { throw UploadToHTTP("while uploading to HTTP binary cache at '%s': %s", cacheUri, e.msg()); } @@ -171,7 +171,7 @@ protected: FileTransferRequest request{makeURI(path)}; try { - return std::move(getFileTransfer()->enqueueFileTransfer(request).get().data); + return std::move(getFileTransfer()->enqueueDownload(request).get().data); } catch (FileTransferError & e) { if (e.error == FileTransfer::NotFound || e.error == FileTransfer::Forbidden) return {}; diff --git a/src/nix/upgrade-nix.cc b/src/nix/upgrade-nix.cc index 791cdab64..3cf868a78 100644 --- a/src/nix/upgrade-nix.cc +++ b/src/nix/upgrade-nix.cc @@ -286,7 +286,7 @@ struct CmdUpgradeNix : MixDryRun, EvalCommand // FIXME: use nixos.org? auto req = FileTransferRequest(storePathsUrl); - auto res = getFileTransfer()->enqueueFileTransfer(req).get(); + auto res = getFileTransfer()->enqueueDownload(req).get(); auto state = std::make_unique(SearchPath{}, store); auto v = state->allocValue(); diff --git a/tests/unit/libstore/filetransfer.cc b/tests/unit/libstore/filetransfer.cc index 1d8d0d5d7..96901aae2 100644 --- a/tests/unit/libstore/filetransfer.cc +++ b/tests/unit/libstore/filetransfer.cc @@ -163,7 +163,7 @@ TEST(FileTransfer, NOT_ON_DARWIN(reportsSetupErrors)) auto [port, srv] = serveHTTP("404 not found", "", [] { return ""; }); auto ft = makeFileTransfer(); ASSERT_THROW( - ft->enqueueFileTransfer(FileTransferRequest(fmt("http://[::1]:%d/index", port))).get(), + ft->enqueueDownload(FileTransferRequest(fmt("http://[::1]:%d/index", port))).get(), FileTransferError ); } @@ -219,7 +219,7 @@ TEST(FileTransfer, usesIntermediateLinkHeaders) auto ft = makeFileTransfer(); FileTransferRequest req(fmt("http://[::1]:%d/first", port)); req.baseRetryTimeMs = 0; - auto result = ft->enqueueFileTransfer(req).get(); + auto result = ft->enqueueDownload(req).get(); ASSERT_EQ(result.immutableUrl, "http://foo"); }