SH-3185 Fill in some FIXME/TODO cases

Also added some comments and changed the callback userdata argument
to be an HttpOpRequest rather than a libcurl handle.  Less code,
less clutter.

7 files changed, 84 insertions, 35 deletions

diff --git a/indra/llcorehttp/_httpopcancel.cpp b/indra/llcorehttp/_httpopcancel.cpp
index 5c1f484109..8e1105dc81 100644
--- a/indra/llcorehttp/_httpopcancel.cpp
+++ b/indra/llcorehttp/_httpopcancel.cpp
@@ -59,6 +59,11 @@ HttpOpCancel::~HttpOpCancel()
 {}
 
 
+// Immediately search for the request on various queues
+// and cancel operations if found.  Return the status of
+// the search and cancel as the status of this request.
+// The canceled request will return a canceled status to
+// its handler.
 void HttpOpCancel::stageFromRequest(HttpService * service)
 {
 	if (! service->cancel(mHandle))
diff --git a/indra/llcorehttp/_httpopcancel.h b/indra/llcorehttp/_httpopcancel.h
index 4d927d1aaf..659d28955f 100644
--- a/indra/llcorehttp/_httpopcancel.h
+++ b/indra/llcorehttp/_httpopcancel.h
@@ -43,9 +43,10 @@ namespace LLCore
 
 
 /// HttpOpCancel requests that a previously issued request
-/// be canceled, if possible.  Requests that have been made
-/// active and are available for sending on the wire cannot
-/// be canceled.  
+/// be canceled, if possible.  This includes active requests
+/// that may be in the middle of an HTTP transaction.  Any
+/// completed request will not be canceled and will return
+/// its final status unchanged.
 
 class HttpOpCancel : public HttpOperation
 {
diff --git a/indra/llcorehttp/_httpoperation.cpp b/indra/llcorehttp/_httpoperation.cpp
index d80a8236e6..910dbf1f2f 100644
--- a/indra/llcorehttp/_httpoperation.cpp
+++ b/indra/llcorehttp/_httpoperation.cpp
@@ -91,31 +91,31 @@ void HttpOperation::setReplyPath(HttpReplyQueue * reply_queue,
 
 void HttpOperation::stageFromRequest(HttpService *)
 {
-	// *FIXME:  Message this a better way later.
 	// Default implementation should never be called.  This
 	// indicates an operation making a transition that isn't
 	// defined.
-	llassert_always(false);
+	LL_ERRS("HttpCore") << "Default stateFromRequest method may not be called."
+						<< LL_ENDL;
 }
 
 
 void HttpOperation::stageFromReady(HttpService *)
 {
-	// *FIXME:  Message this a better way later.
 	// Default implementation should never be called.  This
 	// indicates an operation making a transition that isn't
 	// defined.
-	llassert_always(false);
+	LL_ERRS("HttpCore") << "Default stateFromReady method may not be called."
+						<< LL_ENDL;
 }
 
 
 void HttpOperation::stageFromActive(HttpService *)
 {
-	// *FIXME:  Message this a better way later.
 	// Default implementation should never be called.  This
 	// indicates an operation making a transition that isn't
 	// defined.
-	llassert_always(false);
+	LL_ERRS("HttpCore") << "Default stateFromActive method may not be called."
+						<< LL_ENDL;
 }
 
 
diff --git a/indra/llcorehttp/_httpoprequest.cpp b/indra/llcorehttp/_httpoprequest.cpp
index a19b6bd0dc..bec82d8449 100644
--- a/indra/llcorehttp/_httpoprequest.cpp
+++ b/indra/llcorehttp/_httpoprequest.cpp
@@ -344,6 +344,13 @@ void HttpOpRequest::setupCommon(HttpRequest::policy_t policy_id,
 }
 
 
+// Sets all libcurl options and data for a request.
+//
+// Used both for initial requests and to 'reload' for
+// a retry, generally with a different CURL handle.
+// Junk may be left around from a failed request and that
+// needs to be cleaned out.
+//
 HttpStatus HttpOpRequest::prepareRequest(HttpService * service)
 {
 	// Scrub transport and result data for retried op case
@@ -387,20 +394,29 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service)
 
 	if (ENABLE_LINKSYS_WRT54G_V5_DNS_FIX)
 	{
+		// The Linksys WRT54G V5 router has an issue with frequent
+		// DNS lookups from LAN machines.  If they happen too often,
+		// like for every HTTP request, the router gets annoyed after
+		// about 700 or so requests and starts issuing TCP RSTs to
+		// new connections.  Reuse the DNS lookups for even a few
+		// seconds and no RSTs.
 		curl_easy_setopt(mCurlHandle, CURLOPT_DNS_CACHE_TIMEOUT, 10);
 	}
 	else
 	{
-		// *FIXME:  Revisit this old DNS timeout setting - may no longer be valid
+		// *TODO:  Revisit this old DNS timeout setting - may no longer be valid
+		// I don't think this is valid anymore, the Multi shared DNS
+		// cache is working well.  For the case of naked easy handles,
+		// consider using a shared DNS object.
 		curl_easy_setopt(mCurlHandle, CURLOPT_DNS_CACHE_TIMEOUT, 0);
 	}
 	curl_easy_setopt(mCurlHandle, CURLOPT_AUTOREFERER, 1);
 	curl_easy_setopt(mCurlHandle, CURLOPT_FOLLOWLOCATION, 1);
-	curl_easy_setopt(mCurlHandle, CURLOPT_MAXREDIRS, DEFAULT_HTTP_REDIRECTS);	// *FIXME:  parameterize this later
+	curl_easy_setopt(mCurlHandle, CURLOPT_MAXREDIRS, DEFAULT_HTTP_REDIRECTS);
 	curl_easy_setopt(mCurlHandle, CURLOPT_WRITEFUNCTION, writeCallback);
-	curl_easy_setopt(mCurlHandle, CURLOPT_WRITEDATA, mCurlHandle);
+	curl_easy_setopt(mCurlHandle, CURLOPT_WRITEDATA, this);
 	curl_easy_setopt(mCurlHandle, CURLOPT_READFUNCTION, readCallback);
-	curl_easy_setopt(mCurlHandle, CURLOPT_READDATA, mCurlHandle);
+	curl_easy_setopt(mCurlHandle, CURLOPT_READDATA, this);
 	curl_easy_setopt(mCurlHandle, CURLOPT_SSL_VERIFYPEER, 1);
 	curl_easy_setopt(mCurlHandle, CURLOPT_SSL_VERIFYHOST, 0);
 
@@ -468,7 +484,9 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service)
 		break;
 		
 	default:
-		// *FIXME:  fail out here
+		LL_ERRS("CoreHttp") << "Invalid HTTP method in request:  "
+							<< int(mReqMethod)  << ".  Can't recover."
+							<< LL_ENDL;
 		break;
 	}
 
@@ -476,7 +494,7 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service)
 	if (mTracing >= TRACE_CURL_HEADERS)
 	{
 		curl_easy_setopt(mCurlHandle, CURLOPT_VERBOSE, 1);
-		curl_easy_setopt(mCurlHandle, CURLOPT_DEBUGDATA, mCurlHandle);
+		curl_easy_setopt(mCurlHandle, CURLOPT_DEBUGDATA, this);
 		curl_easy_setopt(mCurlHandle, CURLOPT_DEBUGFUNCTION, debugCallback);
 	}
 	
@@ -524,7 +542,7 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service)
 	if (mProcFlags & (PF_SCAN_RANGE_HEADER | PF_SAVE_HEADERS))
 	{
 		curl_easy_setopt(mCurlHandle, CURLOPT_HEADERFUNCTION, headerCallback);
-		curl_easy_setopt(mCurlHandle, CURLOPT_HEADERDATA, mCurlHandle);
+		curl_easy_setopt(mCurlHandle, CURLOPT_HEADERDATA, this);
 	}
 	
 	if (status)
@@ -537,10 +555,7 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service)
 
 size_t HttpOpRequest::writeCallback(void * data, size_t size, size_t nmemb, void * userdata)
 {
-	CURL * handle(static_cast<CURL *>(userdata));
-	HttpOpRequest * op(NULL);
-	curl_easy_getinfo(handle, CURLINFO_PRIVATE, &op);
-	// *FIXME:  check the pointer
+	HttpOpRequest * op(static_cast<HttpOpRequest *>(userdata));
 
 	if (! op->mReplyBody)
 	{
@@ -554,10 +569,7 @@ size_t HttpOpRequest::writeCallback(void * data, size_t size, size_t nmemb, void
 		
 size_t HttpOpRequest::readCallback(void * data, size_t size, size_t nmemb, void * userdata)
 {
-	CURL * handle(static_cast<CURL *>(userdata));
-	HttpOpRequest * op(NULL);
-	curl_easy_getinfo(handle, CURLINFO_PRIVATE, &op);
-	// *FIXME:  check the pointer
+	HttpOpRequest * op(static_cast<HttpOpRequest *>(userdata));
 
 	if (! op->mReqBody)
 	{
@@ -567,7 +579,8 @@ size_t HttpOpRequest::readCallback(void * data, size_t size, size_t nmemb, void
 	const size_t body_size(op->mReqBody->size());
 	if (body_size <= op->mCurlBodyPos)
 	{
-		// *FIXME:  should probably log this event - unexplained
+		LL_WARNS("HttpCore") << "Request body position beyond body size.  Aborting request."
+							 << LL_ENDL;
 		return 0;
 	}
 
@@ -586,10 +599,7 @@ size_t HttpOpRequest::headerCallback(void * data, size_t size, size_t nmemb, voi
 	static const char con_ran_line[] = "content-range:";
 	static const size_t con_ran_line_len = sizeof(con_ran_line) - 1;
 	
-	CURL * handle(static_cast<CURL *>(userdata));
-	HttpOpRequest * op(NULL);
-	curl_easy_getinfo(handle, CURLINFO_PRIVATE, &op);
-	// *FIXME:  check the pointer
+	HttpOpRequest * op(static_cast<HttpOpRequest *>(userdata));
 
 	const size_t hdr_size(size * nmemb);
 	const char * hdr_data(static_cast<const char *>(data));		// Not null terminated
@@ -609,7 +619,7 @@ size_t HttpOpRequest::headerCallback(void * data, size_t size, size_t nmemb, voi
 	}
 	else if (op->mProcFlags & PF_SCAN_RANGE_HEADER)
 	{
-		char hdr_buffer[128];
+		char hdr_buffer[128];			// Enough for a reasonable header
 		size_t frag_size((std::min)(hdr_size, sizeof(hdr_buffer) - 1));
 		
 		memcpy(hdr_buffer, hdr_data, frag_size);
@@ -638,8 +648,10 @@ size_t HttpOpRequest::headerCallback(void * data, size_t size, size_t nmemb, voi
 			else
 			{
 				// Ignore the unparsable.
-				// *FIXME:  Maybe issue a warning into the log here
-				;
+				LL_INFOS_ONCE("CoreHttp") << "Problem parsing odd Content-Range header:  '"
+										  << std::string(hdr_data, frag_size)
+										  << "'.  Ignoring."
+										  << LL_ENDL;
 			}
 		}
 	}
@@ -668,9 +680,7 @@ size_t HttpOpRequest::headerCallback(void * data, size_t size, size_t nmemb, voi
 
 int HttpOpRequest::debugCallback(CURL * handle, curl_infotype info, char * buffer, size_t len, void * userdata)
 {
-	HttpOpRequest * op(NULL);
-	curl_easy_getinfo(handle, CURLINFO_PRIVATE, &op);
-	// *FIXME:  check the pointer
+	HttpOpRequest * op(static_cast<HttpOpRequest *>(userdata));
 
 	std::string safe_line;
 	std::string tag;
diff --git a/indra/llcorehttp/_httpoprequest.h b/indra/llcorehttp/_httpoprequest.h
index 5d2417466c..a4c5fbb3c2 100644
--- a/indra/llcorehttp/_httpoprequest.h
+++ b/indra/llcorehttp/_httpoprequest.h
@@ -49,6 +49,16 @@ class HttpOptions;
 
 /// HttpOpRequest requests a supported HTTP method invocation with
 /// option and header overrides.
+///
+/// Essentially an RPC to get an HTTP GET, POST or PUT executed
+/// asynchronously with options to override behaviors and HTTP
+/// headers.
+///
+/// Constructor creates a raw object incapable of useful work.
+/// A subsequent call to one of the setupXXX() methods provides
+/// the information needed to make a working request which can
+/// then be enqueued to a request queue.
+///
 
 class HttpOpRequest : public HttpOperation
 {
@@ -177,6 +187,8 @@ public:
 // Free functions
 // ---------------------------------------
 
+// Internal function to append the contents of an HttpHeaders
+// instance to a curl_slist object.
 curl_slist * append_headers_to_slist(const HttpHeaders *, curl_slist * slist);
 
 }   // end namespace LLCore
diff --git a/indra/llcorehttp/_httpopsetpriority.h b/indra/llcorehttp/_httpopsetpriority.h
index 724293ef78..31706b737c 100644
--- a/indra/llcorehttp/_httpopsetpriority.h
+++ b/indra/llcorehttp/_httpopsetpriority.h
@@ -42,6 +42,9 @@ namespace LLCore
 /// searches the various queues looking for a given
 /// request handle and changing it's priority if
 /// found.
+///
+/// *NOTE:  This will very likely be removed in the near future
+/// when priority is removed from the library.
 
 class HttpOpSetPriority : public HttpOperation
 {
diff --git a/indra/llcorehttp/httpcommon.h b/indra/llcorehttp/httpcommon.h
index 42b75edb41..576a113e54 100644
--- a/indra/llcorehttp/httpcommon.h
+++ b/indra/llcorehttp/httpcommon.h
@@ -168,6 +168,24 @@ enum HttpError
 /// a successful status or an error.  The application is responsible
 /// for making that determination and a range like [200, 299] isn't
 /// automatically assumed to be definitive.
+///
+/// Examples:
+///
+/// 1.  Construct a default, successful status code:
+///				HttpStatus();
+///
+/// 2.  Construct a successful, HTTP 200 status code:
+///				HttpStatus(200);
+///
+/// 3.  Construct a failed, HTTP 404 not-found status code:
+///				HttpStatus(404);
+///
+/// 4.  Construct a failed libcurl couldn't connect status code:
+///				HttpStatus(HttpStatus::EXT_CURL_EASY, CURLE_COULDNT_CONNECT);
+///
+/// 5.  Construct an HTTP 301 status code to be treated as success:
+///				HttpStatus(301, HE_SUCCESS);
+///
 
 struct HttpStatus
 {