Skip to contentSkip to conversation

Return CURL error message when object transfer fails

PATCH In review Nick Hengeveld 2 participants 6 messages Jun 15, 2016

lore
Share & tools
lore

Nick Hengeveld

author
#0 lore raw

PATCH 3/3 Return CURL error message when object transfer fails

Return CURL error message when object transfer fails

Signed-off-by Nick Hengeveld <[email protected]>

---

It might be better to extend this to all places that curl_easy_perform is called, rather than just in fetch_object.

 http-fetch.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

330da7634e6a707779dcc8648841f501d2a47568

http-fetch.c +3 -1
@@ -19,6 +19,7 @@
 static CURL *curl;
 static struct curl_slist *no_pragma_header;
 static struct curl_slist *no_range_header;
+static char curl_errorstr[CURL_ERROR_SIZE];
 
 static char *initial_base;
 

@@ -389,6 +390,7 @@ int fetch_object(struct alt_base *repo, 
 	curl_easy_setopt(curl, CURLOPT_FILE, NULL);
 	curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);
 	curl_easy_setopt(curl, CURLOPT_HTTPHEADER, no_pragma_header);
+	curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errorstr);
 
 	url = xmalloc(strlen(repo->base) + 50);
 	strcpy(url, repo->base);

@@ -448,7 +450,7 @@ int fetch_object(struct alt_base *repo, 
 	curl_result = curl_easy_perform(curl);
 	curl_easy_setopt(curl, CURLOPT_HTTPHEADER, no_range_header);
 	if (curl_result != 0) {
-		return -1;
+		return error(curl_errorstr);
 	}
 
 	fchmod(local, 0444);
#1 lore raw
Quoted 2 lines · Nick Hengeveld

It might be better to extend this to all places that curl_easy_perform is called, rather than just in fetch_object.

Sounds like a good idea. Also if you happen to know if this option is not available in older versions of the library, it might not hurt to guard it with "#if LIBCURL_VERSION_NUM" like we do with other options.

author
#2 lore raw
Quoted 4 lines · Junio C Hamano

Sounds like a good idea. Also if you happen to know if this option is not available in older versions of the library, it might not hurt to guard it with "#if LIBCURL_VERSION_NUM" like we do with other options.

I don't see any indication that CURLOPT_ERRORBUFFER is a new feature. The curl_easy_strerror() function is new as of 7.12.0 which is why I elected to use the CURLOPT_ERRORBUFFER option instead.

Signature · 2 lines
For a successful technology, reality must take precedence over public
relations, for nature cannot be fooled.
#3 lore raw
Quoted 3 lines · Nick Hengeveld

I don't see any indication that CURLOPT_ERRORBUFFER is a new feature. The curl_easy_strerror() function is new as of 7.12.0 which is why I elected to use the CURLOPT_ERRORBUFFER option instead.

If that is the case, I'll take your patch verbatim. Thanks.

I just needed to get the feel that you did things that way, knowing what to and what not to worry about; I am no cURL expert.

#4 lore raw
Quoted 1 line · Junio C Hamano

If that is the case, I'll take your patch verbatim. Thanks.

Oops. I spoke too fast. That "sounds like a good idea" of mine was a response to your:

Quoted 2 lines

It might be better to extend this to all places that curl_easy_perform is called, rather than just in fetch_object.

... so the patch still needed some fixups. It had minor dependencies on the previous patches in the series as well, so I tried to fix them up myself.

Could you take a look at it and see if the following is good enough, please?

------------
Subject: [PATCH] Return CURL error message when object transfer fails
From: Nick Hengeveld <[email protected]>
Date: 1127757131 -0700

Return CURL error message when object transfer fails

[jc: added similar curl_errorstr errors to places where we
use curl_easy_perform() to run fetch that _must_ succeed.]

Signed-off-by Nick Hengeveld <[email protected]>
Signed-off-by Junio C Hamano <[email protected]>

---

 http-fetch.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

4f1da6322aa5d11091005b23716bcc3c65151a32

http-fetch.c +16 -8
@@ -15,6 +15,7 @@
 
 static CURL *curl;
 static struct curl_slist *no_pragma_header;
+static char curl_errorstr[CURL_ERROR_SIZE];
 
 static char *initial_base;
 

@@ -112,10 +113,12 @@ static int fetch_index(struct alt_base *
 	curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, fwrite);
 	curl_easy_setopt(curl, CURLOPT_URL, url);
 	curl_easy_setopt(curl, CURLOPT_HTTPHEADER, no_pragma_header);
+	curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errorstr);
 	
 	if (curl_easy_perform(curl)) {
 		fclose(indexfile);
-		return error("Unable to get pack index %s", url);
+		return error("Unable to get pack index %s\n%s", url,
+			     curl_errorstr);
 	}
 
 	fclose(indexfile);

@@ -264,10 +267,10 @@ static int fetch_indices(struct alt_base
 	curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
 	curl_easy_setopt(curl, CURLOPT_URL, url);
 	curl_easy_setopt(curl, CURLOPT_HTTPHEADER, NULL);
+	curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errorstr);
 	
-	if (curl_easy_perform(curl)) {
-		return -1;
-	}
+	if (curl_easy_perform(curl))
+		return error("%s", curl_errorstr);
 
 	while (i < buffer.posn) {
 		switch (data[i]) {

@@ -327,10 +330,12 @@ static int fetch_pack(struct alt_base *r
 	curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, fwrite);
 	curl_easy_setopt(curl, CURLOPT_URL, url);
 	curl_easy_setopt(curl, CURLOPT_HTTPHEADER, no_pragma_header);
-	
+	curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errorstr);
+
 	if (curl_easy_perform(curl)) {
 		fclose(packfile);
-		return error("Unable to get pack file %s", url);
+		return error("Unable to get pack file %s\n%s", url,
+			     curl_errorstr);
 	}
 
 	fclose(packfile);

@@ -373,6 +378,7 @@ int fetch_object(struct alt_base *repo, 
 	curl_easy_setopt(curl, CURLOPT_FILE, NULL);
 	curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);
 	curl_easy_setopt(curl, CURLOPT_HTTPHEADER, no_pragma_header);
+	curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errorstr);
 
 	url = xmalloc(strlen(repo->base) + 50);
 	strcpy(url, repo->base);

@@ -388,7 +394,7 @@ int fetch_object(struct alt_base *repo, 
 
 	if (curl_easy_perform(curl)) {
 		unlink(filename);
-		return -1;
+		return error("%s", curl_errorstr);
 	}
 
 	fchmod(local, 0444);

@@ -453,6 +459,7 @@ int fetch_ref(char *ref, unsigned char *
         curl_easy_setopt(curl, CURLOPT_FILE, &buffer);
         curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
 	curl_easy_setopt(curl, CURLOPT_HTTPHEADER, NULL);
+	curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errorstr);
 
         url = xmalloc(strlen(base) + 6 + strlen(ref));
         strcpy(url, base);

@@ -464,7 +471,8 @@ int fetch_ref(char *ref, unsigned char *
         curl_easy_setopt(curl, CURLOPT_URL, url);
 
         if (curl_easy_perform(curl))
-                return error("Couldn't get %s for %s\n", url, ref);
+                return error("Couldn't get %s for %s\n%s",
+			     url, ref, curl_errorstr);
 
         hex[40] = '\0';
         get_sha1_hex(hex, sha1);
author
#5 lore raw
Quoted 6 lines · Junio C Hamano

... so the patch still needed some fixups. It had minor dependencies on the previous patches in the series as well, so I tried to fix them up myself.

Could you take a look at it and see if the following is good enough, please?

Looks great, thanks.

Quoted 12 lines

------------
Subject: [PATCH] Return CURL error message when object transfer fails
From: Nick Hengeveld <[email protected]>
Date: 1127757131 -0700

Return CURL error message when object transfer fails

[jc: added similar curl_errorstr errors to places where we
use curl_easy_perform() to run fetch that _must_ succeed.]

Signed-off-by Nick Hengeveld <[email protected]>
Signed-off-by Junio C Hamano <[email protected]>
Signature · 2 lines
For a successful technology, reality must take precedence over public
relations, for nature cannot be fooled.