From 0c1bbfe508784e219d8e9f939ddd1bb787e26e4e Mon Sep 17 00:00:00 2001 From: Andrew Lindesay Date: Tue, 31 Jan 2017 20:45:36 +1300 Subject: [PATCH] HaikuDepot: suggested code improvements from Adrien --- .../server/ServerIconExportUpdateProcess.cpp | 78 +++++++++---------- src/apps/haikudepot/server/ServerSettings.cpp | 49 ++++++------ src/apps/haikudepot/server/ServerSettings.h | 15 ++-- .../haikudepot/server/WebAppInterface.cpp | 15 ++-- src/apps/haikudepot/ui/App.cpp | 2 +- .../util/ToFileUrlProtocolListener.cpp | 20 ++++- .../util/ToFileUrlProtocolListener.h | 3 +- 7 files changed, 95 insertions(+), 87 deletions(-) diff --git a/src/apps/haikudepot/server/ServerIconExportUpdateProcess.cpp b/src/apps/haikudepot/server/ServerIconExportUpdateProcess.cpp index 46ed44fe3d..4e6e54c982 100644 --- a/src/apps/haikudepot/server/ServerIconExportUpdateProcess.cpp +++ b/src/apps/haikudepot/server/ServerIconExportUpdateProcess.cpp @@ -10,9 +10,11 @@ #include #include +#include #include #include #include +#include #include #include "ServerSettings.h" @@ -130,8 +132,8 @@ ServerIconExportUpdateProcess::_IfModifiedSinceHeaderValue(BString& headerValue) status_t ServerIconExportUpdateProcess::_Download(BPath& tarGzFilePath) { - BString urlString = ServerSettings::CreateFullUrl("/__pkgicon/all.tar.gz"); - return _Download(tarGzFilePath, BUrl(urlString), 0, 0); + return _Download(tarGzFilePath, + ServerSettings::CreateFullUrl("/__pkgicon/all.tar.gz"), 0, 0); } @@ -152,10 +154,8 @@ ServerIconExportUpdateProcess::_Download(BPath& tarGzFilePath, const BUrl& url, fprintf(stdout, "will stream '%s' to [%s]\n", url.UrlString().String(), tarGzFilePath.Path()); - bool isSecure = url.Protocol() == BString("https"); ToFileUrlProtocolListener listener(tarGzFilePath, "icon-export", ServerSettings::UrlConnectionTraceLoggingEnabled()); - BUrlContext context; BHttpHeaders headers; ServerSettings::AugmentHeaders(headers); @@ -169,55 +169,51 @@ ServerIconExportUpdateProcess::_Download(BPath& tarGzFilePath, const BUrl& url, headers.AddHeader("If-Modified-Since", ifModifiedSinceHeader); } - BHttpRequest request(url, isSecure, "HTTP", &listener, &context); - request.SetMethod(B_HTTP_GET); - request.SetHeaders(headers); - request.SetTimeout(TIMEOUT_MICROSECONDS); + BHttpRequest *request = dynamic_cast( + BUrlProtocolRoster::MakeRequest(url, &listener)); + ObjectDeleter requestDeleter(request); + request->SetHeaders(headers); + request->SetMaxRedirections(0); + request->SetTimeout(TIMEOUT_MICROSECONDS); - thread_id thread = request.Run(); + thread_id thread = request->Run(); wait_for_thread(thread, NULL); const BHttpResult& result = dynamic_cast( - request.Result()); + request->Result()); int32 statusCode = result.StatusCode(); - switch (statusCode) { - case HTTP_STATUS_OK: - fprintf(stdout, "did complete streaming data\n"); - return B_OK; + if (BHttpRequest::IsSuccessStatusCode(statusCode)) { + fprintf(stdout, "did complete streaming data\n"); + return B_OK; + } else if (statusCode == HTTP_STATUS_NOT_MODIFIED) { + fprintf(stdout, "remote data has not changed since [%s]\n", + ifModifiedSinceHeader.String()); + return APP_ERR_NOT_MODIFIED; + } else if (BHttpRequest::IsRedirectionStatusCode(statusCode)) { + const BHttpHeaders responseHeaders = result.Headers(); + const char *locationValue = responseHeaders["Location"]; - case HTTP_STATUS_NOT_MODIFIED: - fprintf(stdout, "remote data has not changed since [%s]\n", - ifModifiedSinceHeader.String()); - return APP_ERR_NOT_MODIFIED; - - case HTTP_STATUS_FOUND: // redirect - { - const BHttpHeaders responseHeaders = result.Headers(); - const char *locationValue = responseHeaders["Location"]; - - if (NULL != locationValue && 0 != strlen(locationValue)) { - BUrl location(locationValue); - fprintf(stdout, "will redirect to; %s\n", - location.UrlString().String()); + if (NULL != locationValue && 0 != strlen(locationValue)) { + BUrl location(locationValue); + fprintf(stdout, "will redirect to; %s\n", + location.UrlString().String()); return _Download(tarGzFilePath, location, redirects + 1, 0); - } - - fprintf(stdout, "unable to find 'Location' header for redirect\n"); - return B_IO_ERROR; } - default: - if (0 == statusCode || 5 == (statusCode / 100)) { - fprintf(stdout, "error response from server; %" B_PRId32 " --> " - "retry...\n", statusCode); - return _Download(tarGzFilePath, url, redirects, failures + 1); - } + fprintf(stdout, "unable to find 'Location' header for redirect\n"); + return B_IO_ERROR; + } else { + if (0 == statusCode || 5 == (statusCode / 100)) { + fprintf(stdout, "error response from server; %" B_PRId32 " --> " + "retry...\n", statusCode); + return _Download(tarGzFilePath, url, redirects, failures + 1); + } - fprintf(stdout, "unexpected response from server; %" B_PRId32 "\n", - statusCode); - return B_IO_ERROR; + fprintf(stdout, "unexpected response from server; %" B_PRId32 "\n", + statusCode); + return B_IO_ERROR; } } diff --git a/src/apps/haikudepot/server/ServerSettings.cpp b/src/apps/haikudepot/server/ServerSettings.cpp index d681120138..b145a6aa16 100644 --- a/src/apps/haikudepot/server/ServerSettings.cpp +++ b/src/apps/haikudepot/server/ServerSettings.cpp @@ -13,74 +13,69 @@ #include #include -#include "AutoLocker.h" - #define BASEURL_DEFAULT "https://depot.haiku-os.org" #define USERAGENT_FALLBACK_VERSION "0.0.0" -BString ServerSettings::fBaseUrl = BString(BASEURL_DEFAULT); -BString ServerSettings::fUserAgent = BString(); -BLocker ServerSettings::fUserAgentLocker; -bool ServerSettings::fUrlConnectionTraceLogging = false; +BUrl ServerSettings::sBaseUrl = BUrl(BASEURL_DEFAULT); +BString ServerSettings::sUserAgent = BString(); +pthread_once_t ServerSettings::sUserAgentInitOnce = PTHREAD_ONCE_INIT; +bool ServerSettings::sUrlConnectionTraceLogging = false; status_t -ServerSettings::SetBaseUrl(const BString& value) +ServerSettings::SetBaseUrl(const BUrl& value) { - BUrl url(value); - - if (!url.IsValid()) { + if (!value.IsValid()) { fprintf(stderr, "the url is not valid\n"); return B_BAD_VALUE; } - if (url.Protocol() != "http" && url.Protocol() != "https") { + if (value.Protocol() != "http" && value.Protocol() != "https") { fprintf(stderr, "the url protocol must be 'http' or 'https'\n"); return B_BAD_VALUE; } - fBaseUrl.SetTo(value); - - if (fBaseUrl.EndsWith("/")) { - fprintf(stderr, "will remove trailing '/' character in url base\n"); - fBaseUrl.Remove(fBaseUrl.Length() - 1, 1); - } + sBaseUrl = value; return B_OK; } -BString +BUrl ServerSettings::CreateFullUrl(const BString urlPathComponents) { - return BString(fBaseUrl) << urlPathComponents; + return BUrl(sBaseUrl, urlPathComponents); } const BString ServerSettings::GetUserAgent() { - AutoLocker lock(&fUserAgentLocker); + if (sUserAgent.IsEmpty()) + pthread_once(&sUserAgentInitOnce, &ServerSettings::_InitUserAgent); - if (fUserAgent.IsEmpty()) { - fUserAgent.SetTo("HaikuDepot/"); - fUserAgent.Append(_GetUserAgentVersionString()); - } + return sUserAgent; +} - return fUserAgent; + +const void +ServerSettings::_InitUserAgent() +{ + sUserAgent.SetTo("HaikuDepot/"); + sUserAgent.Append(_GetUserAgentVersionString()); } void ServerSettings::EnableUrlConnectionTraceLogging() { - fUrlConnectionTraceLogging = true; + sUrlConnectionTraceLogging = true; } bool ServerSettings::UrlConnectionTraceLoggingEnabled() { - return fUrlConnectionTraceLogging; + return sUrlConnectionTraceLogging; } diff --git a/src/apps/haikudepot/server/ServerSettings.h b/src/apps/haikudepot/server/ServerSettings.h index cac2b6e3b2..d3085d403e 100644 --- a/src/apps/haikudepot/server/ServerSettings.h +++ b/src/apps/haikudepot/server/ServerSettings.h @@ -8,27 +8,28 @@ #include #include -#include #include +#include class ServerSettings { public: - static status_t SetBaseUrl(const BString& baseUrl); + static status_t SetBaseUrl(const BUrl& baseUrl); static const BString GetUserAgent(); static void AugmentHeaders(BHttpHeaders& headers); - static BString CreateFullUrl( + static BUrl CreateFullUrl( const BString urlPathComponents); static void EnableUrlConnectionTraceLogging(); static bool UrlConnectionTraceLoggingEnabled(); private: + static const void _InitUserAgent(); static const BString _GetUserAgentVersionString(); - static BString fBaseUrl; - static BString fUserAgent; - static BLocker fUserAgentLocker; - static bool fUrlConnectionTraceLogging; + static BUrl sBaseUrl; + static BString sUserAgent; + static pthread_once_t sUserAgentInitOnce; + static bool sUrlConnectionTraceLogging; }; #endif // SERVER_SETTINGS_H diff --git a/src/apps/haikudepot/server/WebAppInterface.cpp b/src/apps/haikudepot/server/WebAppInterface.cpp index e1b2b7fbdf..4036a64f95 100644 --- a/src/apps/haikudepot/server/WebAppInterface.cpp +++ b/src/apps/haikudepot/server/WebAppInterface.cpp @@ -516,11 +516,11 @@ status_t WebAppInterface::RetrieveScreenshot(const BString& code, int32 width, int32 height, BDataIO* stream) { - BString urlString = ServerSettings::CreateFullUrl(BString("/__pkgscreenshot/") << code - << ".png" << "?tw=" << width << "&th=" << height); - bool isSecure = 0 == urlString.FindFirst("https://"); + BUrl url = ServerSettings::CreateFullUrl( + BString("/__pkgscreenshot/") << code << ".png" << "?tw=" + << width << "&th=" << height); - BUrl url(urlString); + bool isSecure = url.Protocol() == "https"; ProtocolListener listener( ServerSettings::UrlConnectionTraceLoggingEnabled()); @@ -545,7 +545,7 @@ WebAppInterface::RetrieveScreenshot(const BString& code, return B_OK; fprintf(stderr, "failed to get screenshot from '%s': %" B_PRIi32 "\n", - urlString.String(), statusCode); + url.UrlString().String(), statusCode); return B_ERROR; } @@ -629,9 +629,8 @@ WebAppInterface::_SendJsonRequest(const char* domain, BString jsonString, if (ServerSettings::UrlConnectionTraceLoggingEnabled()) printf("_SendJsonRequest(%s)\n", jsonString.String()); - BString urlString = ServerSettings::CreateFullUrl(BString("/__api/v1/") << domain); - bool isSecure = 0 == urlString.FindFirst("https://"); - BUrl url(urlString); + BUrl url = ServerSettings::CreateFullUrl(BString("/__api/v1/") << domain); + bool isSecure = url.Protocol() == "https"; ProtocolListener listener( ServerSettings::UrlConnectionTraceLoggingEnabled()); diff --git a/src/apps/haikudepot/ui/App.cpp b/src/apps/haikudepot/ui/App.cpp index 7578977453..3a8777ab70 100644 --- a/src/apps/haikudepot/ui/App.cpp +++ b/src/apps/haikudepot/ui/App.cpp @@ -197,7 +197,7 @@ App::ArgvReceived(int32 argc, char* argv[]) Quit(); } - if (ServerSettings::SetBaseUrl(argv[i + 1]) != B_OK) { + if (ServerSettings::SetBaseUrl(BUrl(argv[i + 1])) != B_OK) { fprintf(stdout, "malformed web app base url; %s\n", argv[i + 1]); Quit(); diff --git a/src/apps/haikudepot/util/ToFileUrlProtocolListener.cpp b/src/apps/haikudepot/util/ToFileUrlProtocolListener.cpp index 97b57bd30b..e7e68b68ce 100644 --- a/src/apps/haikudepot/util/ToFileUrlProtocolListener.cpp +++ b/src/apps/haikudepot/util/ToFileUrlProtocolListener.cpp @@ -6,6 +6,7 @@ #include "ToFileUrlProtocolListener.h" #include +#include #include @@ -16,6 +17,7 @@ ToFileUrlProtocolListener::ToFileUrlProtocolListener(BPath path, fDownloadIO = new BFile(path.Path(), O_WRONLY | O_CREAT); fTraceLoggingIdentifier = traceLoggingIdentifier; fTraceLogging = traceLogging; + fShouldDownload = true; } @@ -48,6 +50,19 @@ void ToFileUrlProtocolListener::HeadersReceived(BUrlRequest* caller, const BUrlResult& result) { + + // check that the status code is success. Only if it is successful + // should the payload be streamed to the file. + + const BHttpResult& httpResult = dynamic_cast(result); + int32 statusCode = httpResult.StatusCode(); + + if (!BHttpRequest::IsSuccessStatusCode(statusCode)) { + fprintf(stdout, "received %" B_PRId32 + " --> will not store download to file\n", statusCode); + fShouldDownload = false; + } + } @@ -55,12 +70,13 @@ void ToFileUrlProtocolListener::DataReceived(BUrlRequest* caller, const char* data, off_t position, ssize_t size) { - if (fDownloadIO != NULL && size > 0) { + if (fShouldDownload && fDownloadIO != NULL && size > 0) { size_t remaining = size; size_t written = 0; do { - written = fDownloadIO->Write(&data[size - remaining], remaining); + written = fDownloadIO->WriteAt(position, &data[size - remaining], + remaining); remaining -= written; } while (remaining > 0 && written > 0); diff --git a/src/apps/haikudepot/util/ToFileUrlProtocolListener.h b/src/apps/haikudepot/util/ToFileUrlProtocolListener.h index 4b49a107d8..3744a1983f 100644 --- a/src/apps/haikudepot/util/ToFileUrlProtocolListener.h +++ b/src/apps/haikudepot/util/ToFileUrlProtocolListener.h @@ -33,9 +33,10 @@ public: const char* text); private: + bool fShouldDownload; bool fTraceLogging; BString fTraceLoggingIdentifier; - BDataIO* fDownloadIO; + BPositionIO* fDownloadIO; };