git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v4] remote-curl: fix large pushes with GSSAPI

From
Jeff King <peff@peff.net>
Date
Oct 30, 2013, 08:45 UTC
Message-ID
<20131030084510.GO11317@sigill.intra.peff.net>
In-Reply-To
<1383014197-11607-1-git-send-email-sandals@crustytoothpaste.net>
On Tue, Oct 29, 2013 at 02:36:37AM +0000, brian m. carlson wrote:
>  	if (large_request) {
> +		long authtype = 0;
> +

Minor nit, but this will produce an unused variable warning if the code in the #if below doesn't get compiled. I don't know how much we care.

Show 6 quoted lines
> +#if LIBCURL_VERSION_NUM >= 0x070a08
> +		slot = get_active_slot();
> +		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL, &authtype);
> +		if (authtype & CURLAUTH_GSSNEGOTIATE)
> +			needs_100_continue = 1;
> +#endif

I didn't notice this in the last round, but this is somewhat of an abuse of get_active_slot. It is intended to give you a new, pristine curl slot that you can use to make a request, and it is yours until you call finish_active_slot. You're not supposed to look at it after that.

However, we do reuse the curl handles. And in the case of rpc case, we are only doing one request at a time, so the handle you get is guaranteed to be the last one used. So it works in practice, but it would break if the curl handle code breaks any of these assumptions.

I think the clean way to do it would be to teach the slot code to pull out the available auth methods, and pass them up through the call chain. Like this on top of your patch:

diff --git a/http.c b/http.c
index 0ddb164..32fa998 100644
--- a/http.c
+++ b/http.c
@@ -761,6 +761,12 @@ void finish_active_slot(struct active_request_slot *slot)
 	if (slot->results != NULL) {
 		slot->results->curl_result = slot->curl_result;
 		slot->results->http_code = slot->http_code;
+#if LIBCURL_VERSION_NUM >= 0x070a08
+		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL,
+				  &slot->results->authtype);
+#else
+		slot->results->authtype = 0;
+#endif
 	}
 
 	/* Run callback if appropriate */
diff --git a/http.h b/http.h
index d77c1b5..4b32b9b 100644
--- a/http.h
+++ b/http.h
@@ -54,6 +54,7 @@
 struct slot_results {
 	CURLcode curl_result;
 	long http_code;
+	long authtype;
 };
 
 struct active_request_slot {
diff --git a/remote-curl.c b/remote-curl.c
index eaa286c..d026f05 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -383,25 +383,29 @@ static size_t rpc_in(char *ptr, size_t eltsize,
 	return size;
 }
 
-static int run_slot(struct active_request_slot *slot)
+static int run_slot(struct active_request_slot *slot,
+		    struct slot_results *results)
 {
 	int err;
-	struct slot_results results;
+	struct slot_results results_buf;
 
-	slot->results = &results;
+	if (!results)
+		results = &results_buf;
+
+	slot->results = results;
 	slot->curl_result = curl_easy_perform(slot->curl);
 	finish_active_slot(slot);
 
-	err = handle_curl_result(&results);
+	err = handle_curl_result(results);
 	if (err != HTTP_OK && err != HTTP_REAUTH) {
 		error("RPC failed; result=%d, HTTP code = %ld",
-		      results.curl_result, results.http_code);
+		      results->curl_result, results->http_code);
 	}
 
 	return err;
 }
 
-static int probe_rpc(struct rpc_state *rpc)
+static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
 	struct curl_slist *headers = NULL;
@@ -423,7 +427,7 @@ static int probe_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);
 
-	err = run_slot(slot);
+	err = run_slot(slot, results);
 
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);
@@ -462,20 +466,16 @@ static int post_rpc(struct rpc_state *rpc)
 	}
 
 	if (large_request) {
-		long authtype = 0;
+		struct slot_results results;
 
 		do {
-			err = probe_rpc(rpc);
+			err = probe_rpc(rpc, &results);
 		} while (err == HTTP_REAUTH);
 		if (err != HTTP_OK)
 			return -1;
 
-#if LIBCURL_VERSION_NUM >= 0x070a08
-		slot = get_active_slot();
-		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL, &authtype);
-		if (authtype & CURLAUTH_GSSNEGOTIATE)
+		if (results.authtype & CURLAUTH_GSSNEGOTIATE)
 			needs_100_continue = 1;
-#endif
 	}
 
 	headers = curl_slist_append(headers, rpc->hdr_content_type);
@@ -572,7 +572,7 @@ retry:
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
-	err = run_slot(slot);
+	err = run_slot(slot, NULL);
 	if (err == HTTP_REAUTH && !large_request)
 		goto retry;
 	if (err != HTTP_OK)

That's note tested beyond compiling, but I think it should work. Feel
free to squash it into your patch, or if you'd like, I can split out the
refactoring steps with a commit message for you.

-Peff
Previous: brian m. carlsonNext: brian m. carlson
Message 2 of 17 in “remote-curl: fix large pushes with GSSAPI”
  1. remote-curl: fix large pushes with GSSAPIbrian m. carlson, Oct 29, 2013
  2. Jeff KingOct 30, 2013
  3. brian m. carlsonOct 30, 2013
  4. Jeff KingOct 31, 2013
  5. 1/3 http: return curl's AUTHAVAIL via slot_resultsJeff King, Oct 31, 2013
  6. 2/3 remote-curl: pass curl slot_results back through run_slotJeff King, Oct 31, 2013
  7. [BUG?] "error: cache entry has null sha1"Junio C Hamano, Oct 31, 2013
  8. Jeff KingOct 31, 2013
  9. Jeff KingOct 31, 2013
  10. Junio C HamanoOct 31, 2013
  11. Re* [BUG?] "error: cache entry has null sha1"Junio C Hamano, Nov 1, 2013
  12. 1/3 unpack-trees: fix "read-tree -u --reset A B" with conflicted indexJunio C Hamano, Nov 1, 2013
  13. 2/3 t1005: reindentJunio C Hamano, Nov 1, 2013
  14. 3/3 t1005: add test for "read-tree --reset -u A B"Junio C Hamano, Nov 1, 2013
  15. Jeff KingNov 3, 2013
  16. 3/3 remote-curl: fix large pushes with GSSAPIJeff King, Oct 31, 2013
  17. brian m. carlsonOct 31, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.