Print

---

@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

In src/XrdHttp/XrdHttpReq.cc:

> @@ -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.

In src/XrdHttp/XrdHttpReq.cc:

>                        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.

In src/XrdHttp/XrdHttpReq.cc:

> @@ -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?

In src/XrdHttp/XrdHttpReq.cc:

>            // 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?

In src/XrdHttp/XrdHttpReq.cc:

> +        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?

In src/XrdHttp/XrdHttpReq.cc:

> +        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