merge from tridge

(This used to be ctdb commit e83db4d76ea498413ba4314e937d0ff298bd4cb6)

5 files changed, 87 insertions, 42 deletions

diff --git a/ctdb/common/ctdb.c b/ctdb/common/ctdb.c
index a69cbdbee7..60e1e6b90b 100644
--- a/ctdb/common/ctdb.c
+++ b/ctdb/common/ctdb.c
@@ -192,31 +192,39 @@ uint32_t ctdb_get_num_nodes(struct ctdb_context *ctdb)
 */
 void ctdb_recv_pkt(struct ctdb_context *ctdb, uint8_t *data, uint32_t length)
 {
-	struct ctdb_req_header *hdr;
+	struct ctdb_req_header *hdr = (struct ctdb_req_header *)data;
+	TALLOC_CTX *tmp_ctx;
+
+	/* place the packet as a child of the tmp_ctx. We then use
+	   talloc_free() below to free it. If any of the calls want
+	   to keep it, then they will steal it somewhere else, and the
+	   talloc_free() will only free the tmp_ctx */
+	tmp_ctx = talloc_new(ctdb);
+	talloc_steal(tmp_ctx, hdr);
 
 	if (length < sizeof(*hdr)) {
 		ctdb_set_error(ctdb, "Bad packet length %d\n", length);
-		return;
+		goto done;
 	}
-	hdr = (struct ctdb_req_header *)data;
 	if (length != hdr->length) {
 		ctdb_set_error(ctdb, "Bad header length %d expected %d\n", 
 			       hdr->length, length);
-		return;
+		goto done;
 	}
 
 	if (hdr->ctdb_magic != CTDB_MAGIC) {
 		ctdb_set_error(ctdb, "Non CTDB packet rejected\n");
-		return;
+		goto done;
 	}
 
 	if (hdr->ctdb_version != CTDB_VERSION) {
 		ctdb_set_error(ctdb, "Bad CTDB version 0x%x rejected\n", hdr->ctdb_version);
-		return;
+		goto done;
 	}
 
-	DEBUG(3,(__location__ " ctdb request of type %d length %d from node %d to %d\n",
-		 hdr->operation, hdr->length, hdr->srcnode, hdr->destnode));
+	DEBUG(3,(__location__ " ctdb request %d of type %d length %d from "
+		 "node %d to %d\n", hdr->reqid, hdr->operation, hdr->length,
+		 hdr->srcnode, hdr->destnode));
 
 	switch (hdr->operation) {
 	case CTDB_REQ_CALL:
@@ -252,7 +260,9 @@ void ctdb_recv_pkt(struct ctdb_context *ctdb, uint8_t *data, uint32_t length)
 			 __location__, hdr->operation));
 		break;
 	}
-	talloc_free(hdr);
+
+done:
+	talloc_free(tmp_ctx);
 }
 
 /*
diff --git a/ctdb/common/ctdb_call.c b/ctdb/common/ctdb_call.c
index 65a1a5659c..ee8a824fc9 100644
--- a/ctdb/common/ctdb_call.c
+++ b/ctdb/common/ctdb_call.c
@@ -105,6 +105,7 @@ static int ctdb_call_local(struct ctdb_db_context *ctdb_db, struct ctdb_call *ca
 	if (c->reply_data) {
 		call->reply_data = *c->reply_data;
 		talloc_steal(ctdb, call->reply_data.dptr);
+		talloc_set_name_const(call->reply_data.dptr, __location__);
 	} else {
 		call->reply_data.dptr = NULL;
 		call->reply_data.dsize = 0;
@@ -252,6 +253,7 @@ void ctdb_request_dmaster(struct ctdb_context *ctdb, struct ctdb_req_header *hdr
 	struct ctdb_ltdb_header header;
 	struct ctdb_db_context *ctdb_db;
 	int ret, len;
+	TALLOC_CTX *tmp_ctx;
 
 	key.dptr = c->data;
 	key.dsize = c->keylen;
@@ -299,6 +301,12 @@ void ctdb_request_dmaster(struct ctdb_context *ctdb, struct ctdb_req_header *hdr
 	len = offsetof(struct ctdb_reply_dmaster, data) + data.dsize;
 	r = ctdb->methods->allocate_pkt(ctdb, len);
 	CTDB_NO_MEMORY_FATAL(ctdb, r);
+
+	/* put the packet on a temporary context, allowing us to safely free
+	   it below even if ctdb_reply_dmaster() has freed it already */
+	tmp_ctx = talloc_new(ctdb);
+	talloc_steal(tmp_ctx, r);
+
 	talloc_set_name_const(r, "reply_dmaster packet");
 	r->hdr.length    = len;
 	r->hdr.ctdb_magic = CTDB_MAGIC;
@@ -316,7 +324,7 @@ void ctdb_request_dmaster(struct ctdb_context *ctdb, struct ctdb_req_header *hdr
 		ctdb_queue_packet(ctdb, &r->hdr);
 	}
 
-	talloc_free(r);
+	talloc_free(tmp_ctx);
 }
 
 
@@ -414,7 +422,7 @@ void ctdb_request_call(struct ctdb_context *ctdb, struct ctdb_req_header *hdr)
   called when a CTDB_REPLY_CALL packet comes in
 
   This packet comes in response to a CTDB_REQ_CALL request packet. It
-  contains any reply data freom the call
+  contains any reply data from the call
 */
 void ctdb_reply_call(struct ctdb_context *ctdb, struct ctdb_req_header *hdr)
 {
@@ -422,7 +430,10 @@ void ctdb_reply_call(struct ctdb_context *ctdb, struct ctdb_req_header *hdr)
 	struct ctdb_call_state *state;
 
 	state = idr_find(ctdb->idr, hdr->reqid);
-	if (state == NULL) return;
+	if (state == NULL) {
+		DEBUG(0, ("reqid %d not found\n", hdr->reqid));
+		return;
+	}
 
 	if (!talloc_get_type(state, struct ctdb_call_state)) {
 		DEBUG(0,("ctdb idr type error at %s\n", __location__));
@@ -435,10 +446,6 @@ void ctdb_reply_call(struct ctdb_context *ctdb, struct ctdb_req_header *hdr)
 
 	talloc_steal(state, c);
 
-	/* get an extra reference here - this prevents the free in ctdb_recv_pkt()
-	   from freeing the data */
-	(void)talloc_reference(state, c);
-
 	state->state = CTDB_CALL_DONE;
 	if (state->async.fn) {
 		state->async.fn(state);
@@ -488,6 +495,8 @@ void ctdb_reply_dmaster(struct ctdb_context *ctdb, struct ctdb_req_header *hdr)
 
 	ctdb_call_local(ctdb_db, &state->call, &state->header, &data, ctdb->vnn);
 
+	talloc_steal(state, state->call.reply_data.dptr);
+
 	state->state = CTDB_CALL_DONE;
 	if (state->async.fn) {
 		state->async.fn(state);
@@ -619,6 +628,7 @@ struct ctdb_call_state *ctdb_call_local_send(struct ctdb_db_context *ctdb_db,
 	state->ctdb_db = ctdb_db;
 
 	ret = ctdb_call_local(ctdb_db, &state->call, header, data, ctdb->vnn);
+	talloc_steal(state, state->call.reply_data.dptr);
 
 	event_add_timed(ctdb->ev, state, timeval_zero(), call_local_trigger, state);
 
@@ -707,7 +717,10 @@ struct ctdb_call_state *ctdb_daemon_call_send(struct ctdb_db_context *ctdb_db,
 	if (ret != 0) return NULL;
 
 	if (header.dmaster == ctdb->vnn && !(ctdb->flags & CTDB_FLAG_SELF_CONNECT)) {
-		return ctdb_call_local_send(ctdb_db, call, &header, &data);
+		struct ctdb_call_state *state;
+		state = ctdb_call_local_send(ctdb_db, call, &header, &data);
+		talloc_free(data.dptr);
+		return state;
 	}
 
 	talloc_free(data.dptr);
@@ -724,7 +737,7 @@ struct ctdb_call_state *ctdb_daemon_call_send(struct ctdb_db_context *ctdb_db,
 */
 int ctdb_daemon_call_recv(struct ctdb_call_state *state, struct ctdb_call *call)
 {
-	struct ctdb_record_handle *rec;
+	struct ctdb_fetch_handle *rec;
 
 	while (state->state < CTDB_CALL_DONE) {
 		event_loop_once(state->node->ctdb->ev);
@@ -742,6 +755,7 @@ int ctdb_daemon_call_recv(struct ctdb_call_state *state, struct ctdb_call *call)
 		rec->header = state->header;
 		rec->data->dptr = talloc_steal(rec, state->call.reply_data.dptr);
 		rec->data->dsize = state->call.reply_data.dsize;
+		talloc_set_name_const(rec->data->dptr, __location__);
 		talloc_free(state);
 		return 0;
 	}
diff --git a/ctdb/common/ctdb_client.c b/ctdb/common/ctdb_client.c
index 1b5c80c6f4..aca243eda1 100644
--- a/ctdb/common/ctdb_client.c
+++ b/ctdb/common/ctdb_client.c
@@ -75,19 +75,20 @@ void ctdb_reply_fetch_lock(struct ctdb_context *ctdb, struct ctdb_req_header *hd
 	struct ctdb_fetch_lock_state *state;
 
 	state = idr_find(ctdb->idr, hdr->reqid);
-	if (state == NULL) return;
+	if (state == NULL) {
+		DEBUG(0, ("reqid %d not found at %s\n", hdr->reqid,
+			  __location__));
+		return;
+	}
 
 	if (!talloc_get_type(state, struct ctdb_fetch_lock_state)) {
-		DEBUG(0, ("ctdb idr type error at %s\n", __location__));
+		DEBUG(0, ("ctdb idr type error at %s, it's a %s\n",
+			  __location__, talloc_get_name(state)));
 		return;
 	}
 
 	state->r = talloc_steal(state, r);
 
-	/* get an extra reference here - this prevents the free in ctdb_recv_pkt()
-	   from freeing the data */
-	(void)talloc_reference(state, r);
-
 	state->state = CTDB_FETCH_LOCK_DONE;
 }
 
@@ -97,28 +98,35 @@ void ctdb_reply_fetch_lock(struct ctdb_context *ctdb, struct ctdb_req_header *hd
 static void ctdb_client_read_cb(uint8_t *data, size_t cnt, void *args)
 {
 	struct ctdb_context *ctdb = talloc_get_type(args, struct ctdb_context);
-	struct ctdb_req_header *hdr;
+	struct ctdb_req_header *hdr = (struct ctdb_req_header *)data;
+	TALLOC_CTX *tmp_ctx;
+
+	/* place the packet as a child of a tmp_ctx. We then use
+	   talloc_free() below to free it. If any of the calls want
+	   to keep it, then they will steal it somewhere else, and the
+	   talloc_free() will be a no-op */
+	tmp_ctx = talloc_new(ctdb);
+	talloc_steal(tmp_ctx, hdr);
 
 	if (cnt < sizeof(*hdr)) {
 		ctdb_set_error(ctdb, "Bad packet length %d in client\n", cnt);
-		exit(1);
-		return;
+		exit(1); /* XXX - temporary for debugging */
+		goto done;
 	}
-	hdr = (struct ctdb_req_header *)data;
 	if (cnt != hdr->length) {
 		ctdb_set_error(ctdb, "Bad header length %d expected %d in client\n", 
 			       hdr->length, cnt);
-		return;
+		goto done;
 	}
 
 	if (hdr->ctdb_magic != CTDB_MAGIC) {
 		ctdb_set_error(ctdb, "Non CTDB packet rejected in client\n");
-		return;
+		goto done;
 	}
 
 	if (hdr->ctdb_version != CTDB_VERSION) {
 		ctdb_set_error(ctdb, "Bad CTDB version 0x%x rejected in client\n", hdr->ctdb_version);
-		return;
+		goto done;
 	}
 
 	switch (hdr->operation) {
@@ -141,6 +149,9 @@ static void ctdb_client_read_cb(uint8_t *data, size_t cnt, void *args)
 	default:
 		DEBUG(0,("bogus operation code:%d\n",hdr->operation));
 	}
+
+done:
+	talloc_free(tmp_ctx);
 }
 
 /*
@@ -172,6 +183,12 @@ static int ux_socket_connect(struct ctdb_context *ctdb)
 }
 
 
+struct ctdb_record_handle {
+	struct ctdb_db_context *ctdb_db;
+	TDB_DATA key;
+	TDB_DATA *data;
+	struct ctdb_ltdb_header header;
+};
 
 /*
   make a recv call to the local ctdb daemon - called from client context
@@ -260,6 +277,7 @@ struct ctdb_call_state *ctdb_call_send(struct ctdb_db_context *ctdb_db,
 #if 0
 	if (header.dmaster == ctdb->vnn && !(ctdb->flags & CTDB_FLAG_SELF_CONNECT)) {
 		state = ctdb_call_local_send(ctdb_db, call, &header, &data);
+		talloc_free(data.dptr);
 		return state;
 	}
 #endif
@@ -575,7 +593,7 @@ struct ctdb_record_handle *ctdb_fetch_lock(struct ctdb_db_context *ctdb_db, TALL
 
 	talloc_set_destructor(h, fetch_lock_destructor);
 
-	ret = ctdb_ltdb_fetch(ctdb_db, key, &h->header, ctdb_db, data);
+	ret = ctdb_ltdb_fetch(ctdb_db, key, &h->header, h, data);
 	if (ret != 0) {
 		talloc_free(h);
 		return NULL;
diff --git a/ctdb/common/ctdb_daemon.c b/ctdb/common/ctdb_daemon.c
index 94aba21ffb..03cc1777e6 100644
--- a/ctdb/common/ctdb_daemon.c
+++ b/ctdb/common/ctdb_daemon.c
@@ -130,10 +130,10 @@ static struct ctdb_call_state *ctdb_daemon_fetch_lock_send(struct ctdb_db_contex
 							   TDB_DATA *data)
 {
 	struct ctdb_call *call;
-	struct ctdb_record_handle *rec;
+	struct ctdb_fetch_handle *rec;
 	struct ctdb_call_state *state;
 
-	rec = talloc(mem_ctx, struct ctdb_record_handle);
+	rec = talloc(mem_ctx, struct ctdb_fetch_handle);
 	CTDB_NO_MEMORY_NULL(ctdb_db->ctdb, rec);
 
 	
@@ -150,6 +150,7 @@ static struct ctdb_call_state *ctdb_daemon_fetch_lock_send(struct ctdb_db_contex
 
 	state = ctdb_daemon_call_send_remote(ctdb_db, call, header);
 	state->fetch_private = rec;
+	talloc_steal(state, rec);
 
 	return state;
 }
@@ -187,6 +188,7 @@ static void daemon_fetch_lock_complete(struct ctdb_call_state *state)
 		DEBUG(0,(__location__ " Failed to queue packet from daemon to client\n"));
 	}
 	talloc_free(r);
+	talloc_free(state);
 }
 
 /*
@@ -370,6 +372,14 @@ static void daemon_request_call_from_client(struct ctdb_client *client,
 static void daemon_incoming_packet(struct ctdb_client *client, void *data, size_t nread)
 {
 	struct ctdb_req_header *hdr = data;
+	TALLOC_CTX *tmp_ctx;
+
+	/* place the packet as a child of a tmp_ctx. We then use
+	   talloc_free() below to free it. If any of the calls want
+	   to keep it, then they will steal it somewhere else, and the
+	   talloc_free() will be a no-op */
+	tmp_ctx = talloc_new(client);
+	talloc_steal(tmp_ctx, hdr);
 
 	if (hdr->ctdb_magic != CTDB_MAGIC) {
 		ctdb_set_error(client->ctdb, "Non CTDB packet rejected in daemon\n");
@@ -406,7 +416,7 @@ static void daemon_incoming_packet(struct ctdb_client *client, void *data, size_
 	}
 
 done:
-	talloc_free(data);
+	talloc_free(tmp_ctx);
 }
 
 
diff --git a/ctdb/common/ctdb_ltdb.c b/ctdb/common/ctdb_ltdb.c
index 724271da3b..823c296be0 100644
--- a/ctdb/common/ctdb_ltdb.c
+++ b/ctdb/common/ctdb_ltdb.c
@@ -287,13 +287,6 @@ int ctdb_ltdb_lock_fetch_requeue(struct ctdb_db_context *ctdb_db,
 		return -1;
 	}
 
-	/* we get an extra reference to the packet here, to 
-	   stop it being freed in the top level packet handler */
-	if (talloc_reference(ctdb_db, hdr) == NULL) {
-		talloc_free(h);
-		return -1;
-	}
-
 	/* now tell the caller than we will retry asynchronously */
 	return -2;
 }