{"thread":{"id":"3209","subject":"[PATCH 3/9] http-fetch: Abort requests for objects which arrived in packs","startedAt":"2006-02-01T11:28:22Z","lastAt":"2006-02-06T23:11:24Z","messageCount":19,"participants":["Mark Wooding","Nick Hengeveld","Uwe Zeisberger","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"15386","messageId":"20060201112822.5042.41256.stgit@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":null,"subject":"[PATCH 0/9] http-fetch fixes","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-01T11:28:22Z","receivedAt":"2006-02-01T11:28:22Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"This patch series fixes the git-http-fetch bug I reported in\n`git-http-fetch failure/segfault -- alas no patch'.  The preliminary\npatch I was working on /did/ actually fix the bug I found, but uncovered\na bunch more, which I think I've finally got to the bottom of.\n\nThe series applies to commit 1506fc34f7585880aeeb12b5fdfe2de4800f9df5.\n\nParticularly: \n\n  1. watch-slot -- my fix for the actual bug.  Different from Nick\n     Hangeval's separate-status structure, it works by locking slots\n     that people still want answers from.  I've turned the in_use slot\n     field into a bit mask and added a reference counter.  I think you\n     can use Nick's patch instead if you prefer it.\n\n  2, 3. list-corruption, abort-object-request -- fix bugs uncovered by\n     watch-slot.  See the patch emails for the details.\n\n  4, 5, 6, 7. multi-fdset, fix-rename-message, curl-verbose,\n     par-control-flow-tidy -- various other relatively trivial changes I\n     noticed while doing the main work above.  fix-rename-message is\n     actually a bug, but if you think some other fix is better, that's\n     cool; the others are cosmetic things.\n\n  8, 9.  sanity-check-active-slots, sanity-check-object-queue --\n     debugging code I threw in while I was trying to track down the main\n     bugs.  I'm not going to cry if these aren't accepted (indeed, I\n     separated them out and put them at the end specifically!) but they\n     might prove useful to someone, so I thought I'd send 'em along\n     anyway.\n\n-- [mdw]\n"},{"id":"15390","messageId":"20060201114426.5042.97599.stgit@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":"20060201112822.5042.41256.stgit@metalzone.distorted.org.uk","subject":"[PATCH 1/9] http-fetch: Mark slots as `watched' to stop them being reused.","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-01T11:44:26Z","receivedAt":"2006-02-01T11:44:26Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"From: Mark Wooding <mdw@distorted.org.uk>\n\nPreviously, it was possible for the slot passed to run_active_slot() to\nbe reused before that function returned, rendering its completion status\nuseless.  This is particularly noticeable when fetching a repository\nwith a wide, flat directory structure (lots of blobs in the top-level\ntree), a single unpacked commit which changes just one or two blobs, and\nthe rest of the history in one pack.  When fetch_indices() gets the\npack-list, the slot is (usually!) reused for some blob object which is\npacked, so it fails with 404; fetch_indices() decides that the pack-list\nfetch failed, and nothing therefore fetches the pack.\n\nWe fix this by adding a feature to lock slots against reuse.\n\nSigned-off-by: Mark Wooding <mdw@distorted.org.uk>\n---\n\n http-fetch.c |    6 +++---\n http.c       |   43 +++++++++++++++++++++++++++++++++----------\n http.h       |    6 +++++-\n 3 files changed, 41 insertions(+), 14 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex 61b2188..4b1b62d 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -465,13 +465,13 @@ static void process_alternates_response(\n \t\t\t\tbase);\n \t\t\tcurl_easy_setopt(slot->curl, CURLOPT_URL,\n \t\t\t\t\t alt_req->url);\n-\t\t\tactive_requests++;\n-\t\t\tslot->in_use = 1;\n+\t\t\tif (!slot->in_use)\n+\t\t\t\tactive_requests++;\n+\t\t\tslot->in_use |= SLOTUSE_ACTIVE;\n \t\t\tif (start_active_slot(slot)) {\n \t\t\t\treturn;\n \t\t\t} else {\n \t\t\t\tgot_alternates = -1;\n-\t\t\t\tslot->in_use = 0;\n \t\t\t\treturn;\n \t\t\t}\n \t\t}\ndiff --git a/http.c b/http.c\nindex 75e6717..498b4ae 100644\n--- a/http.c\n+++ b/http.c\n@@ -262,7 +262,7 @@ void http_cleanup(void)\n \n \twhile (slot != NULL) {\n #ifdef USE_CURL_MULTI\n-\t\tif (slot->in_use) {\n+\t\tif (slot->in_use & SLOTUSE_ACTIVE) {\n \t\t\tcurl_easy_getinfo(slot->curl,\n \t\t\t\t\t  CURLINFO_EFFECTIVE_URL,\n \t\t\t\t\t  &wait_url);\n@@ -311,6 +311,7 @@ struct active_request_slot *get_active_s\n \t\tnewslot->curl = NULL;\n \t\tnewslot->in_use = 0;\n \t\tnewslot->next = NULL;\n+\t\tnewslot->nrefs = 0;\n \n \t\tslot = active_queue_head;\n \t\tif (slot == NULL) {\n@@ -333,7 +334,7 @@ struct active_request_slot *get_active_s\n \t}\n \n \tactive_requests++;\n-\tslot->in_use = 1;\n+\tslot->in_use |= SLOTUSE_ACTIVE;\n \tslot->local = NULL;\n \tslot->callback_data = NULL;\n \tslot->callback_func = NULL;\n@@ -351,8 +352,9 @@ int start_active_slot(struct active_requ\n \n \tif (curlm_result != CURLM_OK &&\n \t    curlm_result != CURLM_CALL_MULTI_PERFORM) {\n-\t\tactive_requests--;\n-\t\tslot->in_use = 0;\n+\t\tslot->in_use &= ~SLOTUSE_ACTIVE;\n+\t\tif (!slot->in_use)\n+\t\t\tactive_requests--;\n \t\treturn 0;\n \t}\n #endif\n@@ -375,6 +377,24 @@ void step_active_slots(void)\n }\n #endif\n \n+static void watch_active_slot(struct active_request_slot *slot)\n+{\n+\tif (!slot->in_use)\n+\t\tactive_requests++;\n+\tslot->in_use |= SLOTUSE_REF;\n+\tslot->nrefs++;\n+}\n+\n+static void unwatch_active_slot(struct active_request_slot *slot)\n+{\n+\tslot->nrefs--;\n+\tif (!slot->nrefs) {\n+\t\tslot->in_use &= ~SLOTUSE_REF;\n+\t\tif (!slot->in_use)\n+\t\t\tactive_requests--;\n+\t}\n+}\n+\n void run_active_slot(struct active_request_slot *slot)\n {\n #ifdef USE_CURL_MULTI\n@@ -386,7 +406,8 @@ void run_active_slot(struct active_reque\n \tint max_fd;\n \tstruct timeval select_timeout;\n \n-\twhile (slot->in_use) {\n+\twatch_active_slot(slot);\n+\twhile (slot->in_use & SLOTUSE_ACTIVE) {\n \t\tdata_received = 0;\n \t\tstep_active_slots();\n \n@@ -397,7 +418,7 @@ void run_active_slot(struct active_reque\n \t\t\tlast_pos = current_pos;\n \t\t}\n \n-\t\tif (slot->in_use && !data_received) {\n+\t\tif ((slot->in_use & SLOTUSE_ACTIVE) && !data_received) {\n \t\t\tmax_fd = 0;\n \t\t\tFD_ZERO(&readfds);\n \t\t\tFD_ZERO(&writefds);\n@@ -408,8 +429,9 @@ void run_active_slot(struct active_reque\n \t\t\t       &excfds, &select_timeout);\n \t\t}\n \t}\n+\tunwatch_active_slot(slot);\n #else\n-\twhile (slot->in_use) {\n+\twhile (slot->in_use & SLOTUSE_ACTIVE) {\n \t\tslot->curl_result = curl_easy_perform(slot->curl);\n \t\tfinish_active_slot(slot);\n \t}\n@@ -418,8 +440,9 @@ void run_active_slot(struct active_reque\n \n static void finish_active_slot(struct active_request_slot *slot)\n {\n-        active_requests--;\n-        slot->in_use = 0;\n+\tslot->in_use &= ~SLOTUSE_ACTIVE;\n+\tif (!slot->in_use)\n+\t\tactive_requests--;\n         curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);\n  \n         /* Run callback if appropriate */\n@@ -433,7 +456,7 @@ void finish_all_active_slots(void)\n \tstruct active_request_slot *slot = active_queue_head;\n \n \twhile (slot != NULL)\n-\t\tif (slot->in_use) {\n+\t\tif (slot->in_use & SLOTUSE_ACTIVE) {\n \t\t\trun_active_slot(slot);\n \t\t\tslot = active_queue_head;\n \t\t} else {\ndiff --git a/http.h b/http.h\nindex ed4ea33..a66e06a 100644\n--- a/http.h\n+++ b/http.h\n@@ -26,7 +26,8 @@ struct active_request_slot\n {\n \tCURL *curl;\n \tFILE *local;\n-\tint in_use;\n+\tunsigned in_use;\n+\tint nrefs;\n \tCURLcode curl_result;\n \tlong http_code;\n \tvoid *callback_data;\n@@ -34,6 +35,9 @@ struct active_request_slot\n \tstruct active_request_slot *next;\n };\n \n+#define SLOTUSE_ACTIVE 1u\n+#define SLOTUSE_REF 2u\n+\n struct buffer\n {\n         size_t posn;\n"},{"id":"15388","messageId":"20060201114428.5042.53434.stgit@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":"20060201112822.5042.41256.stgit@metalzone.distorted.org.uk","subject":"[PATCH 2/9] http-fetch: Fix object list corruption in fill_active_slots().","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-01T11:44:28Z","receivedAt":"2006-02-01T11:44:28Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"From: Mark Wooding <mdw@distorted.org.uk>\n\nIn fill_active_slots() -- if we find an object which has already arrived,\nsay as part of a pack, /don't/ remove it from the list.  It's already been\nprefetched and someone will ask for it later.  Just label it as done and\ncarry blithely on.  (As it was, the code would dereference a freed object\nto continue through the list anyway.)\n\nSigned-off-by: Mark Wooding <mdw@distorted.org.uk>\n---\n\n http-fetch.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex 4b1b62d..b1ee836 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -312,7 +312,7 @@ void fill_active_slots(void)\n \twhile (active_requests < max_requests && obj_req != NULL) {\n \t\tif (obj_req->state == WAITING) {\n \t\t\tif (has_sha1_file(obj_req->sha1))\n-\t\t\t\trelease_object_request(obj_req);\n+\t\t\t\tobj_req->state = COMPLETE;\n \t\t\telse\n \t\t\t\tstart_object_request(obj_req);\n \t\t\tcurl_multi_perform(curlm, &num_transfers);\n"},{"id":"15385","messageId":"20060201114430.5042.5144.stgit@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":"20060201112822.5042.41256.stgit@metalzone.distorted.org.uk","subject":"[PATCH 3/9] http-fetch: Abort requests for objects which arrived in packs","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-01T11:44:31Z","receivedAt":"2006-02-01T11:44:31Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"From: Mark Wooding <mdw@distorted.org.uk>\n\nIn fetch_object, there's a call to release an object request if the\nobject mysteriously arrived, say in a pack.  Unfortunately, the fetch\nattempt for this object might already be in progress, and we'll leak the\ndescriptor.  Instead, try to tidy away the request.\n\nSigned-off-by: Mark Wooding <mdw@distorted.org.uk>\n---\n\n http-fetch.c |   16 +++++++++++++++-\n http.c       |   20 ++++++++++++++++++--\n http.h       |    1 +\n 3 files changed, 34 insertions(+), 3 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex b1ee836..8656070 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -770,6 +770,20 @@ static int fetch_pack(struct alt_base *r\n \treturn 0;\n }\n \n+static void abort_object_request(struct object_request *obj_req)\n+{\n+\tif (obj_req->local >= 0) {\n+\t\tclose(obj_req->local);\n+\t\tobj_req->local = -1;\n+\t}\n+\tunlink(obj_req->tmpfile);\n+\tif (obj_req->slot) {\n+ \t\trelease_active_slot(obj_req->slot);\n+\t\tobj_req->slot = NULL;\n+\t}\n+\trelease_object_request(obj_req);\n+}\n+\n static int fetch_object(struct alt_base *repo, unsigned char *sha1)\n {\n \tchar *hex = sha1_to_hex(sha1);\n@@ -782,7 +796,7 @@ static int fetch_object(struct alt_base \n \t\treturn error(\"Couldn't find request for %s in the queue\", hex);\n \n \tif (has_sha1_file(obj_req->sha1)) {\n-\t\trelease_object_request(obj_req);\n+\t\tabort_object_request(obj_req);\n \t\treturn 0;\n \t}\n \ndiff --git a/http.c b/http.c\nindex 498b4ae..0a70e1c 100644\n--- a/http.c\n+++ b/http.c\n@@ -438,11 +438,27 @@ void run_active_slot(struct active_reque\n #endif\n }\n \n-static void finish_active_slot(struct active_request_slot *slot)\n+static void closedown_active_slot(struct active_request_slot *slot)\n {\n-\tslot->in_use &= ~SLOTUSE_ACTIVE;\n+        slot->in_use &= ~SLOTUSE_ACTIVE;\n \tif (!slot->in_use)\n \t\tactive_requests--;\n+}\n+\n+void release_active_slot(struct active_request_slot *slot)\n+{\n+\tclosedown_active_slot(slot);\n+\tif (slot->curl) {\n+\t\tcurl_multi_remove_handle(curlm, slot->curl);\n+\t\tcurl_easy_cleanup(slot->curl);\n+\t\tslot->curl = NULL;\n+\t}\n+\tfill_active_slots();\n+}\n+\n+static void finish_active_slot(struct active_request_slot *slot)\n+{\n+\tclosedown_active_slot(slot);\n         curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);\n  \n         /* Run callback if appropriate */\ndiff --git a/http.h b/http.h\nindex a66e06a..d028a5d 100644\n--- a/http.h\n+++ b/http.h\n@@ -58,6 +58,7 @@ extern struct active_request_slot *get_a\n extern int start_active_slot(struct active_request_slot *slot);\n extern void run_active_slot(struct active_request_slot *slot);\n extern void finish_all_active_slots(void);\n+extern void release_active_slot(struct active_request_slot *slot);\n \n #ifdef USE_CURL_MULTI\n extern void fill_active_slots(void);\n"},{"id":"15392","messageId":"20060201114433.5042.22737.stgit@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":"20060201112822.5042.41256.stgit@metalzone.distorted.org.uk","subject":"[PATCH 4/9] http-fetch: Actually watch the file descriptors of interest.","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-01T11:44:33Z","receivedAt":"2006-02-01T11:44:33Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"From: Mark Wooding <mdw@distorted.org.uk>\n\nPresumably this was just some kind of oversight.\n\nSigned-off-by: Mark Wooding <mdw@distorted.org.uk>\n---\n\n http.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 0a70e1c..521323c 100644\n--- a/http.c\n+++ b/http.c\n@@ -425,6 +425,8 @@ void run_active_slot(struct active_reque\n \t\t\tFD_ZERO(&excfds);\n \t\t\tselect_timeout.tv_sec = 0;\n \t\t\tselect_timeout.tv_usec = 50000;\n+\t\t\tcurl_multi_fdset(curlm, &readfds, &writefds,\n+\t\t\t\t\t &excfds, &max_fd);\n \t\t\tselect(max_fd, &readfds, &writefds,\n \t\t\t       &excfds, &select_timeout);\n \t\t}\n"},{"id":"15387","messageId":"20060201114435.5042.11777.stgit@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":"20060201112822.5042.41256.stgit@metalzone.distorted.org.uk","subject":"[PATCH 5/9] http-fetch: Fix message reporting rename of object file.","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-01T11:44:35Z","receivedAt":"2006-02-01T11:44:35Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"From: Mark Wooding <mdw@distorted.org.uk>\n\nmove_temp_to_file returns 0 or -1.  This is not a good thing to pass to\nstrerror(3).  Fortunately, someone already reported the error, so don't\nworry too much.\n\nSigned-off-by: Mark Wooding <mdw@distorted.org.uk>\n---\n\n http-fetch.c |    5 ++---\n 1 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex 8656070..f1aac14 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -831,9 +831,8 @@ static int fetch_object(struct alt_base \n \t} else if (memcmp(obj_req->sha1, obj_req->real_sha1, 20)) {\n \t\tret = error(\"File %s has bad hash\\n\", hex);\n \t} else if (obj_req->rename < 0) {\n-\t\tret = error(\"unable to write sha1 filename %s: %s\",\n-\t\t\t    obj_req->filename,\n-\t\t\t    strerror(obj_req->rename));\n+\t\tret = error(\"unable to write sha1 filename %s\",\n+\t\t\t    obj_req->filename);\n \t}\n \n \trelease_object_request(obj_req);\n"},{"id":"15393","messageId":"20060201114437.5042.53074.stgit@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":"20060201112822.5042.41256.stgit@metalzone.distorted.org.uk","subject":"[PATCH 6/9] http: Turn on verbose Curl messages if GIT_CURL_VERBOSE set in environment","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-01T11:44:37Z","receivedAt":"2006-02-01T11:44:37Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"From: Mark Wooding <mdw@distorted.org.uk>\n\nSigned-off-by: Mark Wooding <mdw@distorted.org.uk>\n---\n\n http.c |    3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 521323c..d0c92dc 100644\n--- a/http.c\n+++ b/http.c\n@@ -192,6 +192,9 @@ static CURL* get_curl_handle(void)\n \n \tcurl_easy_setopt(result, CURLOPT_FOLLOWLOCATION, 1);\n \n+\tif (getenv(\"GIT_CURL_VERBOSE\"))\n+\t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1);\n+\n \treturn result;\n }\n \n"},{"id":"15391","messageId":"20060201114439.5042.20761.stgit@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":"20060201112822.5042.41256.stgit@metalzone.distorted.org.uk","subject":"[PATCH 7/9] http-fetch: Tidy control flow in process_alternate_response","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-01T11:44:39Z","receivedAt":"2006-02-01T11:44:39Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"From: Mark Wooding <mdw@distorted.org.uk>\n\nIt's a bit convoluted.  Tidy it up.\n\nSigned-off-by: Mark Wooding <mdw@distorted.org.uk>\n---\n\n http-fetch.c |    7 ++-----\n 1 files changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex f1aac14..4aa5a11 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -468,12 +468,9 @@ static void process_alternates_response(\n \t\t\tif (!slot->in_use)\n \t\t\t\tactive_requests++;\n \t\t\tslot->in_use |= SLOTUSE_ACTIVE;\n-\t\t\tif (start_active_slot(slot)) {\n-\t\t\t\treturn;\n-\t\t\t} else {\n+\t\t\tif (!start_active_slot(slot))\n \t\t\t\tgot_alternates = -1;\n-\t\t\t\treturn;\n-\t\t\t}\n+\t\t\treturn;\n \t\t}\n \t} else if (slot->curl_result != CURLE_OK) {\n \t\tif (slot->http_code != 404 &&\n"},{"id":"15394","messageId":"20060201114441.5042.13819.stgit@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":"20060201112822.5042.41256.stgit@metalzone.distorted.org.uk","subject":"[PATCH 8/9] http: Paranoid sanity checking for active slots.","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-01T11:44:41Z","receivedAt":"2006-02-01T11:44:41Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"From: Mark Wooding <mdw@distorted.org.uk>\n\nProbably not wanted in the mainline, but very useful when debugging.\n\nSigned-off-by: Mark Wooding <mdw@distorted.org.uk>\n---\n\n http-fetch.c |    2 ++\n http.c       |   28 ++++++++++++++++++++++++++++\n http.h       |    1 +\n 3 files changed, 31 insertions(+), 0 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex 4aa5a11..fa3eb4a 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -465,9 +465,11 @@ static void process_alternates_response(\n \t\t\t\tbase);\n \t\t\tcurl_easy_setopt(slot->curl, CURLOPT_URL,\n \t\t\t\t\t alt_req->url);\n+\t\t\tsanity_check_active_slots();\n \t\t\tif (!slot->in_use)\n \t\t\t\tactive_requests++;\n \t\t\tslot->in_use |= SLOTUSE_ACTIVE;\n+\t\t\tsanity_check_active_slots();\n \t\t\tif (!start_active_slot(slot))\n \t\t\t\tgot_alternates = -1;\n \t\t\treturn;\ndiff --git a/http.c b/http.c\nindex d0c92dc..8087ca0 100644\n--- a/http.c\n+++ b/http.c\n@@ -1,3 +1,4 @@\n+#include <assert.h>\n #include \"http.h\"\n \n int data_received;\n@@ -348,19 +349,36 @@ struct active_request_slot *get_active_s\n \treturn slot;\n }\n \n+void sanity_check_active_slots(void)\n+{\n+\tstruct active_request_slot *slot;\n+\tint n = 0;\n+\n+\tfor (slot = active_queue_head; slot; slot = slot->next) {\n+\t\tassert(!(slot->in_use & SLOTUSE_REF) == !slot->nrefs);\n+\t\tif (slot->in_use)\n+\t\t\tn++;\n+\t}\n+\tassert(n == active_requests);\n+}\n+\n int start_active_slot(struct active_request_slot *slot)\n {\n+\tsanity_check_active_slots();\n #ifdef USE_CURL_MULTI\n \tCURLMcode curlm_result = curl_multi_add_handle(curlm, slot->curl);\n \n \tif (curlm_result != CURLM_OK &&\n \t    curlm_result != CURLM_CALL_MULTI_PERFORM) {\n+\t\tassert(slot->in_use & SLOTUSE_ACTIVE);\n \t\tslot->in_use &= ~SLOTUSE_ACTIVE;\n \t\tif (!slot->in_use)\n \t\t\tactive_requests--;\n+\t\tassert(active_requests >= 0);\n \t\treturn 0;\n \t}\n #endif\n+\tsanity_check_active_slots();\n \treturn 1;\n }\n \n@@ -382,20 +400,26 @@ void step_active_slots(void)\n \n static void watch_active_slot(struct active_request_slot *slot)\n {\n+\tsanity_check_active_slots();\n \tif (!slot->in_use)\n \t\tactive_requests++;\n \tslot->in_use |= SLOTUSE_REF;\n \tslot->nrefs++;\n+\tsanity_check_active_slots();\n }\n \n static void unwatch_active_slot(struct active_request_slot *slot)\n {\n+\tsanity_check_active_slots();\n+\tassert(slot->nrefs);\n \tslot->nrefs--;\n \tif (!slot->nrefs) {\n \t\tslot->in_use &= ~SLOTUSE_REF;\n \t\tif (!slot->in_use)\n \t\t\tactive_requests--;\n+\t\tassert(active_requests >= 0);\n \t}\n+\tsanity_check_active_slots();\n }\n \n void run_active_slot(struct active_request_slot *slot)\n@@ -445,9 +469,13 @@ void run_active_slot(struct active_reque\n \n static void closedown_active_slot(struct active_request_slot *slot)\n {\n+\tsanity_check_active_slots();\n+\tassert(slot->in_use & SLOTUSE_ACTIVE);\n         slot->in_use &= ~SLOTUSE_ACTIVE;\n \tif (!slot->in_use)\n \t\tactive_requests--;\n+\tassert(active_requests >= 0);\n+\tsanity_check_active_slots();\n }\n \n void release_active_slot(struct active_request_slot *slot)\ndiff --git a/http.h b/http.h\nindex d028a5d..a2e48e8 100644\n--- a/http.h\n+++ b/http.h\n@@ -59,6 +59,7 @@ extern int start_active_slot(struct acti\n extern void run_active_slot(struct active_request_slot *slot);\n extern void finish_all_active_slots(void);\n extern void release_active_slot(struct active_request_slot *slot);\n+extern void sanity_check_active_slots(void);\n \n #ifdef USE_CURL_MULTI\n extern void fill_active_slots(void);\n"},{"id":"15389","messageId":"20060201114443.5042.65478.stgit@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":"20060201112822.5042.41256.stgit@metalzone.distorted.org.uk","subject":"[PATCH 9/9] http-fetch: Paranoid sanity checking for the object queue.","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-01T11:44:43Z","receivedAt":"2006-02-01T11:44:43Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"From: Mark Wooding <mdw@distorted.org.uk>\n\nProbably not wanted in the mainline, but useful for debugging.\n\nSigned-off-by: Mark Wooding <mdw@distorted.org.uk>\n---\n\n http-fetch.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 70 insertions(+), 0 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex fa3eb4a..80eaa2f 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -3,6 +3,7 @@\n #include \"pack.h\"\n #include \"fetch.h\"\n #include \"http.h\"\n+#include <assert.h>\n \n #define PREV_BUF_SIZE 4096\n #define RANGE_HEADER_SIZE 30\n@@ -28,6 +29,13 @@ enum object_request_state {\n \tCOMPLETE,\n };\n \n+typedef struct stash {\n+\tstruct stash *next;\n+\tunsigned char sha1[20];\n+\tint activep;\n+\tint tickp;\n+} stash;\n+\n struct object_request\n {\n \tunsigned char sha1[20];\n@@ -47,6 +55,7 @@ struct object_request\n \tint rename;\n \tstruct active_request_slot *slot;\n \tstruct object_request *next;\n+\tstash *tick;\n };\n \n struct alternates_request {\n@@ -57,8 +66,60 @@ struct alternates_request {\n \tint http_specific;\n };\n \n+static stash *seen = 0;\n+\n+static stash *findstash(unsigned char *sha1)\n+{\n+\tstash *s;\n+\tfor (s = seen; s; s = s->next) {\n+\t\tif (memcmp(sha1, s->sha1, 20) == 0)\n+\t\t\treturn s;\n+\t}\n+\treturn NULL;\n+}\n+\n+static stash *enstash(unsigned char *sha1)\n+{\n+\tstash *s;\n+\tif ((s = findstash(sha1)) != NULL)\n+\t\tassert(!s->activep);\n+\telse {\n+\t\ts = xmalloc(sizeof(*s));\n+\t\tmemcpy(s->sha1, sha1, 20);\n+\t\ts->next = seen;\n+\t\tseen = s;\n+\t}\n+\ts->activep = 1;\n+\treturn s;\n+}\n+\n+static void unstash(unsigned char *sha1)\n+{\n+\tstash *s = findstash(sha1);\n+\tassert(s && s->activep);\n+\ts->activep = 0;\n+}\n+\n static struct object_request *object_queue_head = NULL;\n \n+static void sanity_check_stash(void)\n+{\n+\tstash *s;\n+\tstruct object_request *obj_req;\n+\n+\tfor (s = seen; s; s = s->next)\n+\t\ts->tickp = 0;\n+\tfor (obj_req = object_queue_head; obj_req; obj_req = obj_req->next) {\n+\t\tassert(obj_req->tick);\n+\t\ts = obj_req->tick;\n+\t\tassert(memcmp(s->sha1, obj_req->sha1, 20) == 0);\n+\t\tassert(s->activep);\n+\t\ts->tickp = 1;\n+\t}\n+\tfor (s = seen; s; s = s->next)\n+\t\tassert(!s->tickp == !s->activep);\n+}\n+\n static size_t fwrite_sha1_file(void *ptr, size_t eltsize, size_t nmemb,\n \t\t\t       void *data)\n {\n@@ -287,6 +348,7 @@ static void release_object_request(struc\n {\n \tstruct object_request *entry = object_queue_head;\n \n+\tsanity_check_stash();\n \tif (obj_req->local != -1)\n \t\terror(\"fd leakage in release: %d\", obj_req->local);\n \tif (obj_req == object_queue_head) {\n@@ -298,6 +360,8 @@ static void release_object_request(struc\n \t\t\tentry->next = entry->next->next;\n \t}\n \n+\tunstash(obj_req->sha1);\n+\tsanity_check_stash();\n \tfree(obj_req->url);\n \tfree(obj_req);\n }\n@@ -309,6 +373,7 @@ void fill_active_slots(void)\n \tstruct active_request_slot *slot = active_queue_head;\n \tint num_transfers;\n \n+\tsanity_check_stash();\n \twhile (active_requests < max_requests && obj_req != NULL) {\n \t\tif (obj_req->state == WAITING) {\n \t\t\tif (has_sha1_file(obj_req->sha1))\n@@ -327,6 +392,7 @@ void fill_active_slots(void)\n \t\t}\n \t\tslot = slot->next;\n \t}\t\t\t\t\n+\tsanity_check_stash();\n }\n #endif\n \n@@ -347,6 +413,7 @@ void prefetch(unsigned char *sha1)\n \t\t \"%s.temp\", filename);\n \tnewreq->next = NULL;\n \n+\tsanity_check_stash();\n \tif (object_queue_head == NULL) {\n \t\tobject_queue_head = newreq;\n \t} else {\n@@ -356,6 +423,8 @@ void prefetch(unsigned char *sha1)\n \t\t}\n \t\ttail->next = newreq;\n \t}\n+\tnewreq->tick = enstash(sha1);\n+\tsanity_check_stash();\n \n #ifdef USE_CURL_MULTI\n \tfill_active_slots();\n@@ -789,6 +858,7 @@ static int fetch_object(struct alt_base \n \tint ret = 0;\n \tstruct object_request *obj_req = object_queue_head;\n \n+\tsanity_check_stash();\n \twhile (obj_req != NULL && memcmp(obj_req->sha1, sha1, 20))\n \t\tobj_req = obj_req->next;\n \tif (obj_req == NULL)\n"},{"id":"15399","messageId":"20060201150327.GH3873@reactrix.com","threadId":"3209","inReplyTo":"20060201114433.5042.22737.stgit@metalzone.distorted.org.uk","subject":"Re: [PATCH 4/9] http-fetch: Actually watch the file descriptors of interest.","fromName":"Nick Hengeveld","fromEmail":"nickh@reactrix.com","sentAt":"2006-02-01T15:03:27Z","receivedAt":"2006-02-01T15:03:27Z","isPatch":true,"sender":{"key":"nickh@reactrix.com","avatar":null},"body":"On Wed, Feb 01, 2006 at 11:44:33AM +0000, Mark Wooding wrote:\n\n> Presumably this was just some kind of oversight.\n\nI was never able to make this work reliably across multiple versions of\ncurl.\n\n> Signed-off-by: Mark Wooding <mdw@distorted.org.uk>\n> ---\n> \n>  http.c |    2 ++\n>  1 files changed, 2 insertions(+), 0 deletions(-)\n> \n> diff --git a/http.c b/http.c\n> index 0a70e1c..521323c 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -425,6 +425,8 @@ void run_active_slot(struct active_reque\n>  \t\t\tFD_ZERO(&excfds);\n>  \t\t\tselect_timeout.tv_sec = 0;\n>  \t\t\tselect_timeout.tv_usec = 50000;\n> +\t\t\tcurl_multi_fdset(curlm, &readfds, &writefds,\n> +\t\t\t\t\t &excfds, &max_fd);\n>  \t\t\tselect(max_fd, &readfds, &writefds,\n>  \t\t\t       &excfds, &select_timeout);\n>  \t\t}\n> \n> -\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\n-- \nFor a successful technology, reality must take precedence over public\nrelations, for nature cannot be fooled.\n"},{"id":"15400","messageId":"20060201153052.GA16461@informatik.uni-freiburg.de","threadId":"3209","inReplyTo":"20060201112822.5042.41256.stgit@metalzone.distorted.org.uk","subject":"Re: [PATCH 0/9] http-fetch fixes","fromName":"Uwe Zeisberger","fromEmail":"zeisberg@informatik.uni-freiburg.de","sentAt":"2006-02-01T15:30:53Z","receivedAt":"2006-02-01T15:30:53Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello Mark,\n\nMark Wooding wrote:\n> This patch series fixes the git-http-fetch bug I reported in\n> `git-http-fetch failure/segfault -- alas no patch'.  The preliminary\n> patch I was working on /did/ actually fix the bug I found, but uncovered\n> a bunch more, which I think I've finally got to the bottom of.\n> \n> The series applies to commit 1506fc34f7585880aeeb12b5fdfe2de4800f9df5.\nafter reverting c8568e139ed2149fbfb7ef9a8d819d5b6b7c554f it applies to\n8233340ce6eb700eb2cd9c0fef4d1705997c499b (=current master), too.\n    \nWith these patches applied, I get now a Segfault, while cloning u-boot.\n\n        walk a7b9fb9110e3c0be644b3e2c8f397f606138a710\n        got 98dce899a97f7998b11f58e8c7897ba9f7e0b95a\n        got 0cfa422c45d2e2b6023b74a3784034bb18a7eb73\n        got 9341e20e95c5ff5c36a8f9506b4039bbc6aefd43\n        /home/uzeisberger/usr/bin/git-clone: line 42: 24002 Segmentation fault\n        git-http-fetch -v -a -w \"$name\" \"$name\" \"$1/\"\n\nActually now I cannot reproduce it anymore.  I added some debugging code\ns.t. I can give more details if it reoccurs.\n\nBest regards\nUwe\n\n-- \nUwe Zeisberger\n\nhttp://www.google.com/search?q=5+choose+3\n"},{"id":"15403","messageId":"slrndu1m14.2i8.mdw@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":"20060201153052.GA16461@informatik.uni-freiburg.de","subject":"Re: [PATCH 0/9] http-fetch fixes","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-01T15:47:48Z","receivedAt":"2006-02-01T15:47:48Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"Uwe Zeisberger <zeisberg@informatik.uni-freiburg.de> wrote:\n\n>> The series applies to commit 1506fc34f7585880aeeb12b5fdfe2de4800f9df5.\n> after reverting c8568e139ed2149fbfb7ef9a8d819d5b6b7c554f it applies to\n> 8233340ce6eb700eb2cd9c0fef4d1705997c499b (=current master), too.\n\nGood-oh.  (I did pull the latest tip, and then panicked because Nick's\nchange had already made it out.)\n\n> With these patches applied, I get now a Segfault, while cloning\n> u-boot.\n\nOh. :-(  \n\n> Actually now I cannot reproduce it anymore.  I added some debugging code\n> s.t. I can give more details if it reoccurs.\n\nPlease do.  I've just tried, and I can't reproduce the problem either.\n\nThe watch-slots patch seems to uncover all manner of brokenness in\nhttp-fetch.c -- or I've violated some unwritten assumptions of a very\ndeep nature, which is always possible.\n\nMaybe the right thing to do is to go with Nick's separate-status-\nstructures patch as an actual mainline fix, with my patches for\nhttp-fetch.c's other bugs as a belt-and-braces.\n\n-- [mdw]\n"},{"id":"15410","messageId":"20060201171225.GI3873@reactrix.com","threadId":"3209","inReplyTo":"20060201114430.5042.5144.stgit@metalzone.distorted.org.uk","subject":"Re: [PATCH 3/9] http-fetch: Abort requests for objects which arrived in packs","fromName":"Nick Hengeveld","fromEmail":"nickh@reactrix.com","sentAt":"2006-02-01T17:12:25Z","receivedAt":"2006-02-01T17:12:25Z","isPatch":true,"sender":{"key":"nickh@reactrix.com","avatar":null},"body":"On Wed, Feb 01, 2006 at 11:44:31AM +0000, Mark Wooding wrote:\n\n> +void release_active_slot(struct active_request_slot *slot)\n> +{\n> +\tclosedown_active_slot(slot);\n> +\tif (slot->curl) {\n> +\t\tcurl_multi_remove_handle(curlm, slot->curl);\n> +\t\tcurl_easy_cleanup(slot->curl);\n> +\t\tslot->curl = NULL;\n> +\t}\n> +\tfill_active_slots();\n> +}\n\nDoes it make sense to call curl_easy_cleanup here?  This will close\npersistent server connections and cause another connection startup if\nthe slot is reused.\n\n-- \nFor a successful technology, reality must take precedence over public\nrelations, for nature cannot be fooled.\n"},{"id":"15413","messageId":"slrndu1rk9.2i8.mdw@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":"20060201171225.GI3873@reactrix.com","subject":"Re: [PATCH 3/9] http-fetch: Abort requests for objects which arrived in packs","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-01T17:23:21Z","receivedAt":"2006-02-01T17:23:21Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"Nick Hengeveld <nickh@reactrix.com> wrote:\n> Does it make sense to call curl_easy_cleanup here?  This will close\n> persistent server connections and cause another connection startup if\n> the slot is reused.\n\nI'm by no means a Curl expert.  But I scoured the docs for another way\nof interrupting the current fetch and I couldn't find anything.  It\nreally didn't seem like a good idea to leave the handle as it was,\nhaving (possibly) half-fetched a thing we weren't actually interested in\n-- that way lies confusion.\n\nBesides, in practice, what's happened is that the object we asked for is\nin a pack which we've just collected.  Unless the upstream repository\nhasn't been git-prune-packed, our fetch is going to fail with a 404\nanyway, at which point Curl /will/ close the connection and make another\none next time.  So, in real life, it makes no difference.\n\n-- [mdw]\n"},{"id":"15461","messageId":"7vzmlaxy2o.fsf@assigned-by-dhcp.cox.net","threadId":"3209","inReplyTo":"slrndu1m14.2i8.mdw@metalzone.distorted.org.uk","subject":"Re: [PATCH 0/9] http-fetch fixes","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-02-02T03:02:23Z","receivedAt":"2006-02-02T03:02:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Wooding <mdw@distorted.org.uk> writes:\n\n> Uwe Zeisberger <zeisberg@informatik.uni-freiburg.de> wrote:\n>\n>> With these patches applied, I get now a Segfault, while cloning\n>> u-boot.\n> ...\n> Maybe the right thing to do is to go with Nick's separate-status-\n> structures patch as an actual mainline fix, with my patches for\n> http-fetch.c's other bugs as a belt-and-braces.\n\nThanks.\n\nI briefly looked at the series; the following may be ready to be\napplied:\n\n[PATCH 2/9] http-fetch: Fix object list corruption in fill_activ...\n[PATCH 5/9] http-fetch: Fix message reporting rename of object file\n[PATCH 6/9] http: Turn on verbose Curl messages if GIT_CURL_VERB...\n[PATCH 7/9] http-fetch: Tidy control flow in process_alternate_r...\n\nFor now however I'll wait and see for a while, in case you can\nreproduce the breakage Uwe found and add fixes to your set.\n"},{"id":"15527","messageId":"slrndu7enh.2i8.mdw@metalzone.distorted.org.uk","threadId":"3209","inReplyTo":"7vzmlaxy2o.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 0/9] http-fetch fixes","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-02-03T20:20:01Z","receivedAt":"2006-02-03T20:20:01Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"Junio C Hamano <junkio@cox.net> wrote:\n\n> I briefly looked at the series; the following may be ready to be\n> applied:\n>\n> [PATCH 2/9] http-fetch: Fix object list corruption in fill_activ...\n> [PATCH 5/9] http-fetch: Fix message reporting rename of object file\n> [PATCH 6/9] http: Turn on verbose Curl messages if GIT_CURL_VERB...\n> [PATCH 7/9] http-fetch: Tidy control flow in process_alternate_r...\n>\n> For now however I'll wait and see for a while, in case you can\n> reproduce the breakage Uwe found and add fixes to your set.\n\nI've not had any more trouble with this since, but I'm still worried\nthat the http fetcher is rather fragile. :-(\n\nWhat was wrong with 3/9, by the way?  (It's the abort_active_slot patch\n-- which does fix a real bug.)  Is it just that it doesn't apply cleanly\nwithout the first one?  If so, I can send out a new patch easily enough.\n\n-- [mdw]\n"},{"id":"15534","messageId":"7v64nwdvhp.fsf@assigned-by-dhcp.cox.net","threadId":"3209","inReplyTo":"slrndu7enh.2i8.mdw@metalzone.distorted.org.uk","subject":"Re: [PATCH 0/9] http-fetch fixes","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-02-03T20:42:58Z","receivedAt":"2006-02-03T20:42:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Wooding <mdw@distorted.org.uk> writes:\n\n> Junio C Hamano <junkio@cox.net> wrote:\n>\n>> I briefly looked at the series; the following may be ready to be\n>> applied:\n>>\n>> [PATCH 2/9] http-fetch: Fix object list corruption in fill_activ...\n>> [PATCH 5/9] http-fetch: Fix message reporting rename of object file\n>> [PATCH 6/9] http: Turn on verbose Curl messages if GIT_CURL_VERB...\n>> [PATCH 7/9] http-fetch: Tidy control flow in process_alternate_r...\n>>\n>> For now however I'll wait and see for a while, in case you can\n>> reproduce the breakage Uwe found and add fixes to your set.\n>\n> I've not had any more trouble with this since, but I'm still worried\n> that the http fetcher is rather fragile. :-(\n>\n> What was wrong with 3/9, by the way?  (It's the abort_active_slot patch\n> -- which does fix a real bug.)\n\nI saw Nick raised an alternative and you responded but I did not\nsee Nick agreeing or disagreeing to it, and I haven't spent\nenough time on that to convince myself which way to go.  That is\nwhy.\n\nMaybe this weekend.\n"},{"id":"15646","messageId":"20060206231124.GJ3873@reactrix.com","threadId":"3209","inReplyTo":"slrndu1rk9.2i8.mdw@metalzone.distorted.org.uk","subject":"Re: [PATCH 3/9] http-fetch: Abort requests for objects which arrived in packs","fromName":"Nick Hengeveld","fromEmail":"nickh@reactrix.com","sentAt":"2006-02-06T23:11:24Z","receivedAt":"2006-02-06T23:11:24Z","isPatch":true,"sender":{"key":"nickh@reactrix.com","avatar":null},"body":"On Wed, Feb 01, 2006 at 05:23:21PM +0000, Mark Wooding wrote:\n\n> I'm by no means a Curl expert.  But I scoured the docs for another way\n> of interrupting the current fetch and I couldn't find anything.  It\n> really didn't seem like a good idea to leave the handle as it was,\n> having (possibly) half-fetched a thing we weren't actually interested in\n> -- that way lies confusion.\n\nMy impression from the curl documentation was that removing the curl\nhandle from the multi session was enough, but the documentation doesn't\ndescribe what happens to such a request.\n\n> Besides, in practice, what's happened is that the object we asked for is\n> in a pack which we've just collected.  Unless the upstream repository\n> hasn't been git-prune-packed, our fetch is going to fail with a 404\n> anyway, at which point Curl /will/ close the connection and make another\n> one next time.  So, in real life, it makes no difference.\n\nGood point, and it does seem safer to fully clean up the connection\nrather than leave it in a questionable state.\n\n-- \nFor a successful technology, reality must take precedence over public\nrelations, for nature cannot be fooled.\n"}]}