git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Masaya Suzuki <masayasuzuki@google.com>
To: git@vger.kernel.org
Cc: peff@peff.net, jrnieder@gmail.com,
	Masaya Suzuki <masayasuzuki@google.com>
Subject: [PATCH 1/2] Change how HTTP response body is returned
Date: Thu, 27 Dec 2018 17:47:19 -0800	[thread overview]
Message-ID: <20181228014720.206443-1-masayasuzuki@google.com> (raw)

This changes the way HTTP response body is returned in
http_request_reauth and post_rpc.

1. http_request_reauth makes up to two requests; one without a
   credential and one with a credential. The first request can fail if
   it needs a credential. When the keep_error option is specified, the
   response to the first request can be written to the HTTP response
   body destination. If the response body destination is a string
   buffer, it erases the buffer before making the second request. By
   introducing http_response_dest, it can handle the case that the
   destination is a file handle.
2. post_rpc makes an HTTP request and the response body is directly
   written to a file descriptor. This makes it check the HTTP status
   code before writing it, and do not write the response body if it's an
   error response. It's ok without this check now because post_rpc makes
   a request with CURLOPT_FAILONERROR, and libcurl won't call the
   callback if the response has an error status code.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 http.c        | 99 +++++++++++++++++++++++++++++----------------------
 remote-curl.c | 29 ++++++++++++---
 2 files changed, 81 insertions(+), 47 deletions(-)

diff --git a/http.c b/http.c
index eacc2a75e..d23417670 100644
--- a/http.c
+++ b/http.c
@@ -165,6 +165,19 @@ static int http_schannel_check_revoke = 1;
  */
 static int http_schannel_use_ssl_cainfo;
 
+/*
+ * Where to store the result of http_request.
+ *
+ * At most one of buffer or file can be non-NULL. The buffer and file are not
+ * allocated by http_request, and the caller is responsible for releasing them.
+ */
+struct http_response_dest {
+	struct strbuf *buffer;
+
+	FILE *file;
+	const char *filename;
+};
+
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
@@ -1794,12 +1807,8 @@ static void http_opt_request_remainder(CURL *curl, off_t pos)
 	curl_easy_setopt(curl, CURLOPT_RANGE, buf);
 }
 
-/* http_request() targets */
-#define HTTP_REQUEST_STRBUF	0
-#define HTTP_REQUEST_FILE	1
-
 static int http_request(const char *url,
-			void *result, int target,
+			struct http_response_dest *dest,
 			const struct http_get_options *options)
 {
 	struct active_request_slot *slot;
@@ -1812,21 +1821,23 @@ static int http_request(const char *url,
 	slot = get_active_slot();
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
 
-	if (result == NULL) {
-		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
-	} else {
+	if (dest->file) {
+		off_t posn = ftello(dest->file);
 		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
-		curl_easy_setopt(slot->curl, CURLOPT_FILE, result);
-
-		if (target == HTTP_REQUEST_FILE) {
-			off_t posn = ftello(result);
-			curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
-					 fwrite);
-			if (posn > 0)
-				http_opt_request_remainder(slot->curl, posn);
-		} else
-			curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
-					 fwrite_buffer);
+		curl_easy_setopt(slot->curl, CURLOPT_FILE,
+				 dest->file);
+		curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
+				 fwrite);
+		if (posn > 0)
+			http_opt_request_remainder(slot->curl, posn);
+	} else if (dest->buffer) {
+		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
+		curl_easy_setopt(slot->curl, CURLOPT_FILE,
+				 dest->buffer);
+		curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
+				 fwrite_buffer);
+	} else {
+		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
 	}
 
 	accept_language = get_accept_language();
@@ -1930,10 +1941,10 @@ static int update_url_from_redirect(struct strbuf *base,
 }
 
 static int http_request_reauth(const char *url,
-			       void *result, int target,
+			       struct http_response_dest *dest,
 			       struct http_get_options *options)
 {
-	int ret = http_request(url, result, target, options);
+	int ret = http_request(url, dest, options);
 
 	if (ret != HTTP_OK && ret != HTTP_REAUTH)
 		return ret;
@@ -1949,32 +1960,34 @@ static int http_request_reauth(const char *url,
 	if (ret != HTTP_REAUTH)
 		return ret;
 
-	/*
-	 * If we are using KEEP_ERROR, the previous request may have
-	 * put cruft into our output stream; we should clear it out before
-	 * making our next request. We only know how to do this for
-	 * the strbuf case, but that is enough to satisfy current callers.
-	 */
-	if (options && options->keep_error) {
-		switch (target) {
-		case HTTP_REQUEST_STRBUF:
-			strbuf_reset(result);
-			break;
-		default:
-			BUG("HTTP_KEEP_ERROR is only supported with strbufs");
+	if (dest->file) {
+		/*
+		 * At this point, the file contains the response body of the
+		 * previous request. We need to truncate the file.
+		 */
+		FILE *new_file = freopen(dest->filename, "w", dest->file);
+		if (new_file == NULL) {
+			error("Unable to open local file %s", dest->filename);
+			return HTTP_ERROR;
 		}
+		dest->file = new_file;
+	} else if (dest->buffer) {
+		strbuf_reset(dest->buffer);
 	}
 
 	credential_fill(&http_auth);
 
-	return http_request(url, result, target, options);
+	return http_request(url, dest, options);
 }
 
 int http_get_strbuf(const char *url,
-		    struct strbuf *result,
+		    struct strbuf *dest_buffer,
 		    struct http_get_options *options)
 {
-	return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);
+	struct http_response_dest dest;
+	dest.file = NULL;
+	dest.buffer = dest_buffer;
+	return http_request_reauth(url, &dest, options);
 }
 
 /*
@@ -1988,18 +2001,20 @@ static int http_get_file(const char *url, const char *filename,
 {
 	int ret;
 	struct strbuf tmpfile = STRBUF_INIT;
-	FILE *result;
+	struct http_response_dest dest;
 
 	strbuf_addf(&tmpfile, "%s.temp", filename);
-	result = fopen(tmpfile.buf, "a");
-	if (!result) {
+	dest.buffer = NULL;
+	dest.file = fopen(tmpfile.buf, "a");
+	if (!dest.file) {
 		error("Unable to open local file %s", tmpfile.buf);
 		ret = HTTP_ERROR;
 		goto cleanup;
 	}
+	dest.filename = tmpfile.buf;
 
-	ret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);
-	fclose(result);
+	ret = http_request_reauth(url, &dest, options);
+	fclose(dest.file);
 
 	if (ret == HTTP_OK && finalize_object_file(tmpfile.buf, filename))
 		ret = HTTP_ERROR;
diff --git a/remote-curl.c b/remote-curl.c
index 1220dffcd..48656bf18 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -546,14 +546,31 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 }
 #endif
 
+struct rpc_in_data {
+	struct rpc_state *rpc;
+	struct active_request_slot *slot;
+};
+
+/*
+ * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
+ * from ptr.
+ */
 static size_t rpc_in(char *ptr, size_t eltsize,
 		size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
-	struct rpc_state *rpc = buffer_;
+	struct rpc_in_data *data = buffer_;
+	long response_code;
+
+	if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
+			      &response_code) != CURLE_OK)
+		return size;
+	if (response_code != 200)
+		return size;
+
 	if (size)
-		rpc->any_written = 1;
-	write_or_die(rpc->in, ptr, size);
+		data->rpc->any_written = 1;
+	write_or_die(data->rpc->in, ptr, size);
 	return size;
 }
 
@@ -633,6 +650,7 @@ static int post_rpc(struct rpc_state *rpc)
 	size_t gzip_size = 0;
 	int err, large_request = 0;
 	int needs_100_continue = 0;
+	struct rpc_in_data rpc_in_data;
 
 	/* Try to load the entire request, if we can fit it into the
 	 * allocated buffer space we can use HTTP/1.0 and avoid the
@@ -765,8 +783,9 @@ static int post_rpc(struct rpc_state *rpc)
 
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
-
+	rpc_in_data.rpc = rpc;
+	rpc_in_data.slot = slot;
+	curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
 
 	rpc->any_written = 0;
 	err = run_slot(slot, NULL);
-- 
2.20.1.415.g653613c723-goog


             reply	other threads:[~2018-12-28  1:47 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-28  1:47 Masaya Suzuki [this message]
2018-12-28  1:47 ` [PATCH 2/2] Unset CURLOPT_FAILONERROR Masaya Suzuki
2018-12-28 19:36   ` Eric Sunshine
2018-12-28 19:51     ` Masaya Suzuki
2018-12-28 19:58       ` Eric Sunshine
2018-12-28 20:00         ` Masaya Suzuki
2018-12-29 19:44 ` [PATCH v2 0/2] Show HTTP headers of failed requests with GIT_CURL_VERBOSE Masaya Suzuki
2018-12-29 19:44   ` [PATCH v2 1/2] Change how HTTP response body is returned Masaya Suzuki
2019-01-03 19:09     ` Junio C Hamano
2019-01-04 10:11       ` Jeff King
2019-01-04 20:13         ` Junio C Hamano
2019-01-04 10:30     ` Jeff King
2018-12-29 19:44   ` [PATCH v2 2/2] Unset CURLOPT_FAILONERROR Masaya Suzuki
2019-01-04 10:49     ` Jeff King
2019-01-07 23:24       ` Masaya Suzuki
2019-01-08  2:47   ` [PATCH v3 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE Masaya Suzuki
2019-01-08  2:47     ` [PATCH v3 1/5] http: support file handles for HTTP_KEEP_ERROR Masaya Suzuki
2019-01-09 12:15       ` SZEDER Gábor
2019-01-08  2:47     ` [PATCH v3 2/5] http: enable keep_error for HTTP requests Masaya Suzuki
2019-01-08  2:47     ` [PATCH v3 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION Masaya Suzuki
2019-01-08  2:47     ` [PATCH v3 4/5] remote-curl: unset CURLOPT_FAILONERROR Masaya Suzuki
2019-01-08  2:47     ` [PATCH v3 5/5] test: test GIT_CURL_VERBOSE=1 shows an error Masaya Suzuki
2019-01-10 19:33     ` [PATCH v4 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 1/5] http: support file handles for HTTP_KEEP_ERROR Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 2/5] http: enable keep_error for HTTP requests Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 4/5] remote-curl: unset CURLOPT_FAILONERROR Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 5/5] test: test GIT_CURL_VERBOSE=1 shows an error Masaya Suzuki
2019-01-10 23:06       ` [PATCH v4 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181228014720.206443-1-masayasuzuki@google.com \
    --to=masayasuzuki@google.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).