From abf8df869c79ee6fa021b117e60683fe149131d8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 12 Oct 2012 03:35:33 -0400 Subject: remote-curl: do not call run_slot repeatedly Commit b81401c (http: prompt for credentials on failed POST) taught post_rpc to call run_slot in a loop in order to retry a request after asking the user for credentials. However, after a call to run_slot we will have called finish_active_slot. This means we have released the slot, and we should no longer look at it. As it happens, this does not cause any bugs in the current code, since we know that we are not using curl_multi in this code path, and therefore nobody will have taken over our slot in the meantime. However, it is good form to actually call get_active_slot again. It also future proofs us against changes in the http code. We can do this by jumping back to a retry label at the top of our function. We just need to reorder a few setup lines that should not be repeated; everything else within the loop is either idempotent, needs to be repeated, or in a path we do not follow (e.g., we do not even try when large_request is set, because we don't know how much data we might have streamed from our helper program). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote-curl.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) (limited to 'remote-curl.c') diff --git a/remote-curl.c b/remote-curl.c index 6054e4792..4281262c7 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -444,6 +444,11 @@ static int post_rpc(struct rpc_state *rpc) return -1; } + headers = curl_slist_append(headers, rpc->hdr_content_type); + headers = curl_slist_append(headers, rpc->hdr_accept); + headers = curl_slist_append(headers, "Expect:"); + +retry: slot = get_active_slot(); curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0); @@ -451,10 +456,6 @@ static int post_rpc(struct rpc_state *rpc) curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url); curl_easy_setopt(slot->curl, CURLOPT_ENCODING, ""); - headers = curl_slist_append(headers, rpc->hdr_content_type); - headers = curl_slist_append(headers, rpc->hdr_accept); - headers = curl_slist_append(headers, "Expect:"); - if (large_request) { /* The request body is large and the size cannot be predicted. * We must use chunked encoding to send it. @@ -528,9 +529,9 @@ static int post_rpc(struct rpc_state *rpc) curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in); curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc); - do { - err = run_slot(slot); - } while (err == HTTP_REAUTH && !large_request && !use_gzip); + err = run_slot(slot); + if (err == HTTP_REAUTH && !large_request && !use_gzip) + goto retry; if (err != HTTP_OK) err = -1; -- cgit v1.2.1 From 1960897ebc5a899a8e4ec3c2afc1d2325574fe41 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 12 Oct 2012 03:35:59 -0400 Subject: http: do not set up curl auth after a 401 When we get an http 401, we prompt for credentials and put them in our global credential struct. We also feed them to the curl handle that produced the 401, with the intent that they will be used on a retry. When the code was originally introduced in commit 42653c0, this was a necessary step. However, since dfa1725, we always feed our global credential into every curl handle when we initialize the slot with get_active_slot. So every further request already feeds the credential to curl. Moreover, accessing the slot here is somewhat dubious. After the slot has produced a response, we don't actually control it any more. If we are using curl_multi, it may even have been re-initialized to handle a different request. It just so happens that we will reuse the curl handle within the slot in such a case, and that because we only keep one global credential, it will be the one we want. So the current code is not buggy, but it is misleading. By cleaning it up, we can remove the slot argument entirely from handle_curl_result, making it much more obvious that slots should not be accessed after they are marked as finished. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote-curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'remote-curl.c') diff --git a/remote-curl.c b/remote-curl.c index 4281262c7..aefafd33d 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -369,7 +369,7 @@ static int run_slot(struct active_request_slot *slot) slot->curl_result = curl_easy_perform(slot->curl); finish_active_slot(slot); - err = handle_curl_result(slot, &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); -- cgit v1.2.1