@bbockelm requested changes on this pull request.
Hi Justas!
Looks great! Thanks for the contribution! A few minor items, mostly on styling. I'm happy with the approach, of course, because it's quite similar to what is done in XrdTpc
.
Brian
> @@ -1013,6 +1017,18 @@ int XrdHttpReq::ProcessHTTPReq() { } } + // + // Set log information only if not set + // + if (!prot->rec.paramsSet) { + prot->rec.paramsSet = true; + prot->rec.filename = resource.c_str(); + prot->rec.remote = prot->SecEntity.host; + prot->rec.name = prot->SecEntity.name;
SecEntity.name
is potentially a nullptr
. If it's an unauthenticated client (plain HTTP), this will segfault.
> prot->SendSimpleResp(200, NULL, NULL, (char *) mydata->data, mydata->len, keepalive); reset(); return keepalive ? 1 : -1; } } -
Please avoid non-essential whitespace movement in a commit with functional changes.... can you undo these and squash?
In src/XrdHttp/XrdHttpProtocol.cc:
> @@ -184,6 +184,7 @@ SecEntity(""), CurrentReq(this) { Addr_str = 0; Reset(); ishttps = imhttps; + rec = LogRecord();
Shouldn't the reset of rec
be in the Reset()
function? That function is called whenever the XrdHttpProtocol
object is reused for future, unrelated requests.
> @@ -1013,6 +1017,18 @@ int XrdHttpReq::ProcessHTTPReq() { } } + // + // Set log information only if not set + // + if (!prot->rec.paramsSet) { + prot->rec.paramsSet = true; + prot->rec.filename = resource.c_str();
What c_str()
here instead off the copy constructor?
> // Do a Stat if (prot->doStat((char *) resourceplusopaque.c_str())) { XrdOucString errmsg = "Error stating"; errmsg += resource.c_str(); + logTransferEvent(TRACE_STATSALL, 404,(char *) errmsg.c_str());
Why the const-cast here? That suggests maybe the definition of logTransferEvent
is wrong?
> + return "MOVE"; + case XrdHttpReq::rtPOST: + return "POST"; + default: + return "NOT_RECOGNIZED"; + } +} + + +void XrdHttpReq::logTransferEvent(const int debug, const int retval, const char *message) +{ + if (retval != -1) + prot->rec.status = retval; + std::stringstream ss; + ss << "retval="<< prot->rec.status << ", filename=" << prot->rec.filename; + ss << ", user=" << prot->rec.name << ", reqtype=" << prot->rec.reqtype;
Can we skip the user=
record when rec.name
is empty?
> + return "PROPFIND"; + case XrdHttpReq::rtMKCOL: + return "MKCOL"; + case XrdHttpReq::rtMOVE: + return "MOVE"; + case XrdHttpReq::rtPOST: + return "POST"; + default: + return "NOT_RECOGNIZED"; + } +} + + +void XrdHttpReq::logTransferEvent(const int debug, const int retval, const char *message) +{ + if (retval != -1)
Can we short circuit the creation of the std::stringstream
if all debugging is disabled?.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
Use REPLY-ALL to reply to list
To unsubscribe from the XROOTD-DEV list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1