@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?
> @@ -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 or view it on GitHub:
https://github.com/xrootd/xrootd/pull/1536#pullrequestreview-780124772
########################################################################
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
