-
Notifications
You must be signed in to change notification settings - Fork 37.7k
httpserver: use own HTTP status codes #18168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Nice find and welcome as a contributor! Concept ACK In addition to $ for S in $(git grep '>WriteReply(' | tr "()," "\n" | egrep ^HTTP_ | sort -u); do
echo "=== $S"
grep "$S" src/rpc/protocol.h
echo
done
=== HTTP_BADMETHOD
=== HTTP_BAD_METHOD
HTTP_BAD_METHOD = 405,
=== HTTP_FORBIDDEN
HTTP_FORBIDDEN = 403,
=== HTTP_INTERNAL
HTTP_INTERNAL_SERVER_ERROR = 500,
=== HTTP_NOTFOUND
=== HTTP_OK
HTTP_OK = 200,
=== HTTP_UNAUTHORIZED
HTTP_UNAUTHORIZED = 401,
|
Thanks! I've stumbled upon this while debugging a client that exceeds the HTTP worker queue. I'll cleanup the remaining status codes. |
bf4a595
to
aff2748
Compare
Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file. Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations.
@toxeus To get some structure in the handling of status code we might want to consider changing from Something along the lines of ... Click for patchdiff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index c085095a2..b9eeff819 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -388,7 +388,7 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
responseErrorMessage = strprintf(" (error code %d - \"%s\")", response.error, http_errorstring(response.error));
}
throw CConnectionFailed(strprintf("Could not connect to the server %s:%d%s\n\nMake sure the bitcoind server is running and that you are connecting to the correct RPC port.", host, port, responseErrorMessage));
- } else if (response.status == HTTP_UNAUTHORIZED) {
+ } else if (response.status == static_cast<int>(HTTPStatusCode::HTTP_UNAUTHORIZED)) {
if (failedToGetAuthCookie) {
throw std::runtime_error(strprintf(
"Could not locate RPC credentials. No authentication cookie could be found, and RPC password is not set. See -rpcpassword and -stdinrpcpass. Configuration file: (%s)",
@@ -396,7 +396,7 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
} else {
throw std::runtime_error("Authorization failed: Incorrect rpcuser or rpcpassword");
}
- } else if (response.status >= 400 && response.status != HTTP_BAD_REQUEST && response.status != HTTP_NOT_FOUND && response.status != HTTP_INTERNAL_SERVER_ERROR)
+ } else if (response.status >= 400 && response.status != static_cast<int>(HTTPStatusCode::HTTP_BAD_REQUEST) && response.status != static_cast<int>(HTTPStatusCode::HTTP_NOT_FOUND) && response.status != static_cast<int>(HTTPStatusCode::HTTP_INTERNAL_SERVER_ERROR))
throw std::runtime_error(strprintf("server returned HTTP error %d", response.status));
else if (response.body.empty())
throw std::runtime_error("no response from server");
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index ff7578922..ec5734997 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -76,13 +76,13 @@ static bool g_rpc_whitelist_default = false;
static void JSONErrorReply(HTTPRequest* req, const UniValue& objError, const UniValue& id)
{
// Send error reply from json-rpc error object
- int nStatus = HTTP_INTERNAL_SERVER_ERROR;
+ HTTPStatusCode nStatus = HTTPStatusCode::HTTP_INTERNAL_SERVER_ERROR;
int code = find_value(objError, "code").get_int();
if (code == RPC_INVALID_REQUEST)
- nStatus = HTTP_BAD_REQUEST;
+ nStatus = HTTPStatusCode::HTTP_BAD_REQUEST;
else if (code == RPC_METHOD_NOT_FOUND)
- nStatus = HTTP_NOT_FOUND;
+ nStatus = HTTPStatusCode::HTTP_NOT_FOUND;
std::string strReply = JSONRPCReply(NullUniValue, objError, id);
@@ -155,14 +155,14 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
{
// JSONRPC handles only POST
if (req->GetRequestMethod() != HTTPRequest::POST) {
- req->WriteReply(HTTP_BAD_METHOD, "JSONRPC server handles only POST requests");
+ req->WriteReply(HTTPStatusCode::HTTP_BAD_METHOD, "JSONRPC server handles only POST requests");
return false;
}
// Check authorization
std::pair<bool, std::string> authHeader = req->GetHeader("authorization");
if (!authHeader.first) {
req->WriteHeader("WWW-Authenticate", WWW_AUTH_HEADER_DATA);
- req->WriteReply(HTTP_UNAUTHORIZED);
+ req->WriteReply(HTTPStatusCode::HTTP_UNAUTHORIZED);
return false;
}
@@ -177,7 +177,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
MilliSleep(250);
req->WriteHeader("WWW-Authenticate", WWW_AUTH_HEADER_DATA);
- req->WriteReply(HTTP_UNAUTHORIZED);
+ req->WriteReply(HTTPStatusCode::HTTP_UNAUTHORIZED);
return false;
}
@@ -194,7 +194,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
bool user_has_whitelist = g_rpc_whitelist.count(jreq.authUser);
if (!user_has_whitelist && g_rpc_whitelist_default) {
LogPrintf("RPC User %s not allowed to call any methods\n", jreq.authUser);
- req->WriteReply(HTTP_FORBIDDEN);
+ req->WriteReply(HTTPStatusCode::HTTP_FORBIDDEN);
return false;
// singleton request
@@ -202,7 +202,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
jreq.parse(valRequest);
if (user_has_whitelist && !g_rpc_whitelist[jreq.authUser].count(jreq.strMethod)) {
LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, jreq.strMethod);
- req->WriteReply(HTTP_FORBIDDEN);
+ req->WriteReply(HTTPStatusCode::HTTP_FORBIDDEN);
return false;
}
UniValue result = tableRPC.execute(jreq);
@@ -222,7 +222,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
std::string strMethod = find_value(request, "method").get_str();
if (!g_rpc_whitelist[jreq.authUser].count(strMethod)) {
LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, strMethod);
- req->WriteReply(HTTP_FORBIDDEN);
+ req->WriteReply(HTTPStatusCode::HTTP_FORBIDDEN);
return false;
}
}
@@ -234,7 +234,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
throw JSONRPCError(RPC_PARSE_ERROR, "Top-level object parse error");
req->WriteHeader("Content-Type", "application/json");
- req->WriteReply(HTTP_OK, strReply);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, strReply);
} catch (const UniValue& objError) {
JSONErrorReply(req, objError, jreq.id);
return false;
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 0e13b8580..54f0d077c 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -228,7 +228,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
if (!ClientAllowed(hreq->GetPeer())) {
LogPrint(BCLog::HTTP, "HTTP request from %s rejected: Client network is not allowed RPC access\n",
hreq->GetPeer().ToString());
- hreq->WriteReply(HTTP_FORBIDDEN);
+ hreq->WriteReply(HTTPStatusCode::HTTP_FORBIDDEN);
return;
}
@@ -236,7 +236,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
if (hreq->GetRequestMethod() == HTTPRequest::UNKNOWN) {
LogPrint(BCLog::HTTP, "HTTP request from %s rejected: Unknown HTTP request method\n",
hreq->GetPeer().ToString());
- hreq->WriteReply(HTTP_BADMETHOD);
+ hreq->WriteReply(HTTPStatusCode::HTTP_BAD_METHOD);
return;
}
@@ -268,10 +268,10 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
item.release(); /* if true, queue took ownership */
else {
LogPrintf("WARNING: request rejected because http work queue depth exceeded, it can be increased with the -rpcworkqueue= setting\n");
- item->req->WriteReply(HTTP_INTERNAL, "Work queue depth exceeded");
+ item->req->WriteReply(HTTPStatusCode::HTTP_INTERNAL_SERVER_ERROR, "Work queue depth exceeded");
}
} else {
- hreq->WriteReply(HTTP_NOTFOUND);
+ hreq->WriteReply(HTTPStatusCode::HTTP_NOT_FOUND);
}
}
@@ -519,7 +519,7 @@ HTTPRequest::~HTTPRequest()
if (!replySent) {
// Keep track of whether reply was sent to avoid request leaks
LogPrintf("%s: Unhandled request\n", __func__);
- WriteReply(HTTP_INTERNAL, "Unhandled request");
+ WriteReply(HTTPStatusCode::HTTP_INTERNAL_SERVER_ERROR, "Unhandled request");
}
// evhttpd cleans up the request, as long as a reply was sent.
}
@@ -567,7 +567,7 @@ void HTTPRequest::WriteHeader(const std::string& hdr, const std::string& value)
* Replies must be sent in the main loop in the main http thread,
* this cannot be done from worker threads.
*/
-void HTTPRequest::WriteReply(int nStatus, const std::string& strReply)
+void HTTPRequest::WriteReply(HTTPStatusCode nStatus, const std::string& strReply)
{
assert(!replySent && req);
if (ShutdownRequested()) {
@@ -579,7 +579,7 @@ void HTTPRequest::WriteReply(int nStatus, const std::string& strReply)
evbuffer_add(evb, strReply.data(), strReply.size());
auto req_copy = req;
HTTPEvent* ev = new HTTPEvent(eventBase, true, [req_copy, nStatus]{
- evhttp_send_reply(req_copy, nStatus, nullptr, nullptr);
+ evhttp_send_reply(req_copy, static_cast<int>(nStatus), nullptr, nullptr);
// Re-enable reading from the socket. This is the second part of the libevent
// workaround above.
if (event_get_version_number() >= 0x02010600 && event_get_version_number() < 0x02020001) {
diff --git a/src/httpserver.h b/src/httpserver.h
index 46820e6ae..f9710f844 100644
--- a/src/httpserver.h
+++ b/src/httpserver.h
@@ -5,6 +5,8 @@
#ifndef BITCOIN_HTTPSERVER_H
#define BITCOIN_HTTPSERVER_H
+#include <rpc/protocol.h>
+
#include <string>
#include <functional>
@@ -112,7 +114,7 @@ public:
* @note Can be called only once. As this will give the request back to the
* main thread, do not call any other HTTPRequest methods after calling this.
*/
- void WriteReply(int nStatus, const std::string& strReply = "");
+ void WriteReply(HTTPStatusCode nStatus, const std::string& strReply = "");
};
/** Event handler closure.
diff --git a/src/rest.cpp b/src/rest.cpp
index 062955758..cab7226a1 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -83,7 +83,7 @@ static bool RESTERR(HTTPRequest* req, enum HTTPStatusCode status, std::string me
static CTxMemPool* GetMemPool(HTTPRequest* req)
{
if (!g_rpc_node || !g_rpc_node->mempool) {
- RESTERR(req, HTTP_NOT_FOUND, "Mempool disabled or instance not found");
+ RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "Mempool disabled or instance not found");
return nullptr;
}
return g_rpc_node->mempool;
@@ -130,7 +130,7 @@ static bool CheckWarmup(HTTPRequest* req)
{
std::string statusmessage;
if (RPCIsInWarmup(&statusmessage))
- return RESTERR(req, HTTP_SERVICE_UNAVAILABLE, "Service temporarily unavailable: " + statusmessage);
+ return RESTERR(req, HTTPStatusCode::HTTP_SERVICE_UNAVAILABLE, "Service temporarily unavailable: " + statusmessage);
return true;
}
@@ -145,16 +145,16 @@ static bool rest_headers(HTTPRequest* req,
boost::split(path, param, boost::is_any_of("/"));
if (path.size() != 2)
- return RESTERR(req, HTTP_BAD_REQUEST, "No header count specified. Use /rest/headers/<count>/<hash>.<ext>.");
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "No header count specified. Use /rest/headers/<count>/<hash>.<ext>.");
long count = strtol(path[0].c_str(), nullptr, 10);
if (count < 1 || count > 2000)
- return RESTERR(req, HTTP_BAD_REQUEST, "Header count out of range: " + path[0]);
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Header count out of range: " + path[0]);
std::string hashStr = path[1];
uint256 hash;
if (!ParseHashStr(hashStr, hash))
- return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
const CBlockIndex* tip = nullptr;
std::vector<const CBlockIndex *> headers;
@@ -180,7 +180,7 @@ static bool rest_headers(HTTPRequest* req,
std::string binaryHeader = ssHeader.str();
req->WriteHeader("Content-Type", "application/octet-stream");
- req->WriteReply(HTTP_OK, binaryHeader);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, binaryHeader);
return true;
}
@@ -192,7 +192,7 @@ static bool rest_headers(HTTPRequest* req,
std::string strHex = HexStr(ssHeader.begin(), ssHeader.end()) + "\n";
req->WriteHeader("Content-Type", "text/plain");
- req->WriteReply(HTTP_OK, strHex);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, strHex);
return true;
}
case RetFormat::JSON: {
@@ -202,11 +202,11 @@ static bool rest_headers(HTTPRequest* req,
}
std::string strJSON = jsonHeaders.write() + "\n";
req->WriteHeader("Content-Type", "application/json");
- req->WriteReply(HTTP_OK, strJSON);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
return true;
}
default: {
- return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: .bin, .hex)");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: .bin, .hex)");
}
}
}
@@ -222,7 +222,7 @@ static bool rest_block(HTTPRequest* req,
uint256 hash;
if (!ParseHashStr(hashStr, hash))
- return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
CBlock block;
CBlockIndex* pblockindex = nullptr;
@@ -232,14 +232,14 @@ static bool rest_block(HTTPRequest* req,
tip = ::ChainActive().Tip();
pblockindex = LookupBlockIndex(hash);
if (!pblockindex) {
- return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, hashStr + " not found");
}
if (IsBlockPruned(pblockindex))
- return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, hashStr + " not available (pruned data)");
if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus()))
- return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, hashStr + " not found");
}
switch (rf) {
@@ -248,7 +248,7 @@ static bool rest_block(HTTPRequest* req,
ssBlock << block;
std::string binaryBlock = ssBlock.str();
req->WriteHeader("Content-Type", "application/octet-stream");
- req->WriteReply(HTTP_OK, binaryBlock);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, binaryBlock);
return true;
}
@@ -257,7 +257,7 @@ static bool rest_block(HTTPRequest* req,
ssBlock << block;
std::string strHex = HexStr(ssBlock.begin(), ssBlock.end()) + "\n";
req->WriteHeader("Content-Type", "text/plain");
- req->WriteReply(HTTP_OK, strHex);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, strHex);
return true;
}
@@ -265,12 +265,12 @@ static bool rest_block(HTTPRequest* req,
UniValue objBlock = blockToJSON(block, tip, pblockindex, showTxDetails);
std::string strJSON = objBlock.write() + "\n";
req->WriteHeader("Content-Type", "application/json");
- req->WriteReply(HTTP_OK, strJSON);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
return true;
}
default: {
- return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
}
}
}
@@ -302,11 +302,11 @@ static bool rest_chaininfo(HTTPRequest* req, const std::string& strURIPart)
UniValue chainInfoObject = getblockchaininfo(jsonRequest);
std::string strJSON = chainInfoObject.write() + "\n";
req->WriteHeader("Content-Type", "application/json");
- req->WriteReply(HTTP_OK, strJSON);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
return true;
}
default: {
- return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: json)");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: json)");
}
}
}
@@ -326,11 +326,11 @@ static bool rest_mempool_info(HTTPRequest* req, const std::string& strURIPart)
std::string strJSON = mempoolInfoObject.write() + "\n";
req->WriteHeader("Content-Type", "application/json");
- req->WriteReply(HTTP_OK, strJSON);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
return true;
}
default: {
- return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: json)");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: json)");
}
}
}
@@ -349,11 +349,11 @@ static bool rest_mempool_contents(HTTPRequest* req, const std::string& strURIPar
std::string strJSON = mempoolObject.write() + "\n";
req->WriteHeader("Content-Type", "application/json");
- req->WriteReply(HTTP_OK, strJSON);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
return true;
}
default: {
- return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: json)");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: json)");
}
}
}
@@ -367,7 +367,7 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
uint256 hash;
if (!ParseHashStr(hashStr, hash))
- return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
if (g_txindex) {
g_txindex->BlockUntilSyncedToCurrentChain();
@@ -376,7 +376,7 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
CTransactionRef tx;
uint256 hashBlock = uint256();
if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock))
- return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, hashStr + " not found");
switch (rf) {
case RetFormat::BINARY: {
@@ -385,7 +385,7 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
std::string binaryTx = ssTx.str();
req->WriteHeader("Content-Type", "application/octet-stream");
- req->WriteReply(HTTP_OK, binaryTx);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, binaryTx);
return true;
}
@@ -395,7 +395,7 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
std::string strHex = HexStr(ssTx.begin(), ssTx.end()) + "\n";
req->WriteHeader("Content-Type", "text/plain");
- req->WriteReply(HTTP_OK, strHex);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, strHex);
return true;
}
@@ -404,12 +404,12 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
TxToUniv(*tx, hashBlock, objTx);
std::string strJSON = objTx.write() + "\n";
req->WriteHeader("Content-Type", "application/json");
- req->WriteReply(HTTP_OK, strJSON);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
return true;
}
default: {
- return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
}
}
}
@@ -431,7 +431,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
// throw exception in case of an empty request
std::string strRequestMutable = req->ReadBody();
if (strRequestMutable.length() == 0 && uriParts.size() == 0)
- return RESTERR(req, HTTP_BAD_REQUEST, "Error: empty request");
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Error: empty request");
bool fInputParsed = false;
bool fCheckMemPool = false;
@@ -453,7 +453,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
std::string strOutput = uriParts[i].substr(uriParts[i].find('-')+1);
if (!ParseInt32(strOutput, &nOutput) || !IsHex(strTxid))
- return RESTERR(req, HTTP_BAD_REQUEST, "Parse error");
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Parse error");
txid.SetHex(strTxid);
vOutPoints.push_back(COutPoint(txid, (uint32_t)nOutput));
@@ -462,7 +462,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
if (vOutPoints.size() > 0)
fInputParsed = true;
else
- return RESTERR(req, HTTP_BAD_REQUEST, "Error: empty request");
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Error: empty request");
}
switch (rf) {
@@ -478,7 +478,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
if (strRequestMutable.size() > 0)
{
if (fInputParsed) //don't allow sending input over URI and HTTP RAW DATA
- return RESTERR(req, HTTP_BAD_REQUEST, "Combination of URI scheme inputs and raw post data is not allowed");
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Combination of URI scheme inputs and raw post data is not allowed");
CDataStream oss(SER_NETWORK, PROTOCOL_VERSION);
oss << strRequestMutable;
@@ -487,24 +487,24 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
}
} catch (const std::ios_base::failure&) {
// abort in case of unreadable binary data
- return RESTERR(req, HTTP_BAD_REQUEST, "Parse error");
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Parse error");
}
break;
}
case RetFormat::JSON: {
if (!fInputParsed)
- return RESTERR(req, HTTP_BAD_REQUEST, "Error: empty request");
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Error: empty request");
break;
}
default: {
- return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
}
}
// limit max outpoints
if (vOutPoints.size() > MAX_GETUTXOS_OUTPOINTS)
- return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Error: max outpoints exceeded (max: %d, tried: %d)", MAX_GETUTXOS_OUTPOINTS, vOutPoints.size()));
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, strprintf("Error: max outpoints exceeded (max: %d, tried: %d)", MAX_GETUTXOS_OUTPOINTS, vOutPoints.size()));
// check spentness and form a bitmap (as well as a JSON capable human-readable string representation)
std::vector<unsigned char> bitmap;
@@ -551,7 +551,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
std::string ssGetUTXOResponseString = ssGetUTXOResponse.str();
req->WriteHeader("Content-Type", "application/octet-stream");
- req->WriteReply(HTTP_OK, ssGetUTXOResponseString);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, ssGetUTXOResponseString);
return true;
}
@@ -561,7 +561,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
std::string strHex = HexStr(ssGetUTXOResponse.begin(), ssGetUTXOResponse.end()) + "\n";
req->WriteHeader("Content-Type", "text/plain");
- req->WriteReply(HTTP_OK, strHex);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, strHex);
return true;
}
@@ -591,11 +591,11 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
// return json string
std::string strJSON = objGetUTXOResponse.write() + "\n";
req->WriteHeader("Content-Type", "application/json");
- req->WriteReply(HTTP_OK, strJSON);
+ req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
return true;
}
default: {
- return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
}
}
}
@@ -609,14 +609,14 @@ static bool rest_blockhash_by_height(HTTPRequest* req,
int32_t blockheight;
if (!ParseInt32(height_str, &blockheight) || blockheight < 0) {
- return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
+ return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
}
CBlockIndex* pblockindex = nullptr;
{
LOCK(cs_main);
if (blockheight > ::ChainActive().Height()) {
- return RESTERR(req, HTTP_NOT_FOUND, "Block height out of range");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "Block height out of range");
}
pblockindex = ::ChainActive()[blockheight];
}
@@ -625,23 +625,23 @@ static bool rest_blockhash_by_height(HTTPRequest* req,
CDataStream ss_blockhash(SER_NETWORK, PROTOCOL_VERSION);
ss_blockhash << pblockindex->GetBlockHash();
req->WriteHeader("Content-Type", "application/octet-stream");
- req->WriteReply(HTTP_OK, ss_blockhash.str());
+ req->WriteReply(HTTPStatusCode::HTTP_OK, ss_blockhash.str());
return true;
}
case RetFormat::HEX: {
req->WriteHeader("Content-Type", "text/plain");
- req->WriteReply(HTTP_OK, pblockindex->GetBlockHash().GetHex() + "\n");
+ req->WriteReply(HTTPStatusCode::HTTP_OK, pblockindex->GetBlockHash().GetHex() + "\n");
return true;
}
case RetFormat::JSON: {
req->WriteHeader("Content-Type", "application/json");
UniValue resp = UniValue(UniValue::VOBJ);
resp.pushKV("blockhash", pblockindex->GetBlockHash().GetHex());
- req->WriteReply(HTTP_OK, resp.write() + "\n");
+ req->WriteReply(HTTPStatusCode::HTTP_OK, resp.write() + "\n");
return true;
}
default: {
- return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
+ return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
}
}
}
diff --git a/src/rpc/protocol.h b/src/rpc/protocol.h
index d1475f452..23d6e94b6 100644
--- a/src/rpc/protocol.h
+++ b/src/rpc/protocol.h
@@ -7,7 +7,7 @@
#define BITCOIN_RPC_PROTOCOL_H
//! HTTP status codes
-enum HTTPStatusCode
+enum class HTTPStatusCode
{
HTTP_OK = 200,
HTTP_BAD_REQUEST = 400, |
@practicalswift your suggestion increases type safety therefore I like it. But the suggested change is logically independent from this change therefore I'd prefer to make a follow up PR for that. |
ACK aff2748 -- patch looks correct |
I'd rather we go the opposite direction, and adopt libevent's macros (eliminating our own). Is there a reason to have our own duplicated macros? Or are they just holdovers from the pre-libevent implementation? |
In general, enums confer more type information, passing more information further down into the compile pipeline, vs macros. All other things being equal, that lends weight to a enums>macros preference, for greater -O0 debug-ability -- symbol names in gdb, vs numbers -- as well as greater type clarity & safety. |
Agree with @jgarzik. This is what the C++ Core Guidelines has to say:
|
If we go with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK adding new enum class
and dropping #include <rpc/protocol.h>
from src/httpserver.cpp
.
@promag your statement is self-contradictory. The enum in discussion is in |
@toxeus yes I know, I think it should be in |
Makes sense. Thanks for clarifying 👍 |
HTTP status codes aren't really an enum, though... |
enums are used often beyond the strict semantics you're referring to. They group constants together that belong together. enum classes even add type safety. The fact that the HTTP status codes are defined in an enum since years and nobody seemed to care supports my argument I think. Still, I'm also not happy about duplicating here something that is also defined in libevent. OTOH, the added type safety and improved code readability do justify a duplication using an enum class. |
You're right. They are a protocol & specification from RFC 2616 and others, which can be implemented as a macro, enum, JSON database, or some other method which best suits the situation. |
ACK aff2748 |
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov) Pull request description: Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file. Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations. ACKs for top commit: practicalswift: ACK aff2748 -- patch looks correct laanwj: ACK aff2748 Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
- rpc/wallet: initialize nFeeRequired to avoid using garbage value on failure bitcoin#18274 - httpserver: use own HTTP status codes bitcoin#18168 - tests: Add fuzzing harnesses for various Base{32,58,64} and hex related functions bitcoin#17229 - util: Don't allow Base32/64-decoding or ParseMoney(…) on strings with embedded NUL characters. Add tests. bitcoin#17753 - util: Fail to parse empty string in ParseMoney bitcoin#18225 - util: Fail to parse whitespace-only strings in ParseMoney(...) (instead of parsing as zero) bitcoin#18270 - Replace the LogPrint function with a macro bitcoin#17218 - Fix wallet unload race condition bitcoin#18338 - qt: Fix Window -> Minimize menu item bitcoin#18549 - windows: Enable heap terminate-on-corruption bitcoin#17916
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov) Pull request description: Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file. Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations. ACKs for top commit: practicalswift: ACK aff2748 -- patch looks correct laanwj: ACK aff2748 Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
Summary: Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file. Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations. This is a backport of Core [[bitcoin/bitcoin#18168 | PR18168]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8779
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov) Pull request description: Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file. Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations. ACKs for top commit: practicalswift: ACK aff2748 -- patch looks correct laanwj: ACK aff2748 Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov) Pull request description: Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file. Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations. ACKs for top commit: practicalswift: ACK aff2748 -- patch looks correct laanwj: ACK aff2748 Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov) Pull request description: Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file. Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations. ACKs for top commit: practicalswift: ACK aff2748 -- patch looks correct laanwj: ACK aff2748 Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov) Pull request description: Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file. Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations. ACKs for top commit: practicalswift: ACK aff2748 -- patch looks correct laanwj: ACK aff2748 Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov) Pull request description: Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file. Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations. ACKs for top commit: practicalswift: ACK aff2748 -- patch looks correct laanwj: ACK aff2748 Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov) Pull request description: Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file. Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations. ACKs for top commit: practicalswift: ACK aff2748 -- patch looks correct laanwj: ACK aff2748 Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
Before, macros defined in
<event2/http.h>
have been used for some HTTP status codes.<event2/http.h>
is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.Now, the
HTTPStatusCode
enum from<rpc/protocol.h>
is consistently used for all HTTP response implementations.