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.
Nice find and welcome as a contributor!
Concept ACK
In addition to HTTP_INTERNAL it seems like we have the same issue also for HTTP_BADMETHOD and HTTP_NOTFOUND?
$ 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.
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 enum HTTPStatusCode to enum class HTTPStatusCode?
Something along the lines of ...
<details><summary>Click for patch</summary>
diff --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](/bitcoin-bitcoin/contributor/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,
</details>
@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 aff2748f8aee72f03b5fb6f6f97f0d0f66391755 -- 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:
enum class is strictly better than plain enumenum is strictly better than macro (#define)#define) is strictly better than magic number
If we go with enum over libevent macros, why not also change type to HTTPStatusCode everywhere (for example, line int nStatus = HTTP_INTERNAL_SERVER_ERROR; in src/httprpc.cpp)?
Concept ACK adding new enum class and dropping #include <rpc/protocol.h> from src/httpserver.cpp.
Makes sense. Thanks for clarifying :+1:
HTTP status codes aren't really an enum, though...
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.
HTTP status codes aren't really an enum, though...
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 aff2748f8aee72f03b5fb6f6f97f0d0f66391755
This slightly improves consistency. I do not think converting status codes to an enum class is necessary. As said by others they're well-standardized numbers, not arbitrary enum items.