From: Andrew Tridgell <tridge@samba.org>
Date: Wed, 18 Apr 2007 01:20:24 +0000 (+1000)
Subject: - merge volkers debug changes
X-Git-Tag: tevent-0.9.20~348^2~2887
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=8f059f4d91ee8df59eb9ae4052ca8f5afa392c3b;p=thirdparty%2Fsamba.git

- merge volkers debug changes

- fixed memory leaks in the 3 packet receive routines. The problem was
  that the ctdb_call logic would occasionally complete and free a
  incoming packet, which would then be freed again in the packet
  receive routine. The solution is to make the packet a child of a
  temporary context in the receive routine then free that temporary
  context. That allows other routines to keep or free the packet if
  they want to, while allowing us to safely free it (via a free of the
  temporary context) in the receive function

(This used to be ctdb commit 304aaaa7235febbe97ff9ecb43875b7265ac48cd)
---

8f059f4d91ee8df59eb9ae4052ca8f5afa392c3b
diff --cc ctdb/common/ctdb.c
index a69cbdbee72,57ebd131da1..60e1e6b90b4
--- a/ctdb/common/ctdb.c
+++ b/ctdb/common/ctdb.c
@@@ -192,31 -192,32 +192,39 @@@ uint32_t ctdb_get_num_nodes(struct 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 -253,7 +260,9 @@@
  			 __location__, hdr->operation));
  		break;
  	}
--	talloc_free(hdr);
++
++done:
++	talloc_free(tmp_ctx);
  }
  
  /*
diff --cc ctdb/common/ctdb_call.c
index 65a1a5659c9,aa88829fa1c..ee8a824fc9b
--- a/ctdb/common/ctdb_call.c
+++ b/ctdb/common/ctdb_call.c
@@@ -105,6 -105,6 +105,7 @@@ static int ctdb_call_local(struct ctdb_
  	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 -252,6 +253,7 @@@ void ctdb_request_dmaster(struct ctdb_c
  	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 -299,6 +301,12 @@@
  	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 -316,7 +324,7 @@@
  		ctdb_queue_packet(ctdb, &r->hdr);
  	}
  
--	talloc_free(r);
++	talloc_free(tmp_ctx);
  }
  
  
@@@ -433,12 -436,12 +444,8 @@@ void ctdb_reply_call(struct ctdb_contex
  	state->call.reply_data.dsize = c->datalen;
  	state->call.status = c->status;
  
 -	talloc_steal(c, state);
 -
 -	/* get an extra reference here - this prevents the free in ctdb_recv_pkt()
 -	   from freeing the data */
 -	(void)talloc_reference(state, c);
 +	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 -491,6 +495,8 @@@ void ctdb_reply_dmaster(struct ctdb_con
  
  	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 -622,6 +628,7 @@@ struct ctdb_call_state *ctdb_call_local
  	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 -710,10 +717,10 @@@ struct ctdb_call_state *ctdb_daemon_cal
  	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 *result;
 -		result = 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 result;
++		return state;
  	}
  
  	talloc_free(data.dptr);
@@@ -724,7 -730,7 +737,7 @@@
  */
  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 -748,6 +755,7 @@@
  		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 --cc ctdb/common/ctdb_client.c
index 1b5c80c6f47,d14647ba905..aca243eda12
--- a/ctdb/common/ctdb_client.c
+++ b/ctdb/common/ctdb_client.c
@@@ -84,10 -89,10 +89,6 @@@ void ctdb_reply_fetch_lock(struct ctdb_
  
  	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 -102,28 +98,35 @@@
  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 -146,6 +149,9 @@@
  	default:
  		DEBUG(0,("bogus operation code:%d\n",hdr->operation));
  	}
++
++done:
++	talloc_free(tmp_ctx);
  }
  
  /*
@@@ -172,6 -177,6 +183,12 @@@ static int ux_socket_connect(struct ctd
  }
  
  
++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 -265,6 +277,7 @@@ struct ctdb_call_state *ctdb_call_send(
  #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
diff --cc ctdb/common/ctdb_daemon.c
index 94aba21ffb1,94aba21ffb1..03cc1777e61
--- a/ctdb/common/ctdb_daemon.c
+++ b/ctdb/common/ctdb_daemon.c
@@@ -130,10 -130,10 +130,10 @@@ static struct ctdb_call_state *ctdb_dae
  							   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,6 +150,7 @@@
  
  	state = ctdb_daemon_call_send_remote(ctdb_db, call, header);
  	state->fetch_private = rec;
++	talloc_steal(state, rec);
  
  	return state;
  }
@@@ -187,6 -187,6 +188,7 @@@ static void daemon_fetch_lock_complete(
  		DEBUG(0,(__location__ " Failed to queue packet from daemon to client\n"));
  	}
  	talloc_free(r);
++	talloc_free(state);
  }
  
  /*
@@@ -370,6 -370,6 +372,14 @@@ static void daemon_request_call_from_cl
  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 -406,7 +416,7 @@@
  	}
  
  done:
--	talloc_free(data);
++	talloc_free(tmp_ctx);
  }
  
  
diff --cc ctdb/common/ctdb_ltdb.c
index 724271da3bd,724271da3bd..823c296be0d
--- a/ctdb/common/ctdb_ltdb.c
+++ b/ctdb/common/ctdb_ltdb.c
@@@ -287,13 -287,13 +287,6 @@@ int ctdb_ltdb_lock_fetch_requeue(struc
  		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;
  }
diff --cc ctdb/include/ctdb_private.h
index be0fbf85ae0,be0fbf85ae0..6736ee4ab7c
--- a/ctdb/include/ctdb_private.h
+++ b/ctdb/include/ctdb_private.h
@@@ -195,7 -195,7 +195,7 @@@ struct ctdb_call_state 
  
  
  /* used for fetch_lock */
--struct ctdb_record_handle {
++struct ctdb_fetch_handle {
  	struct ctdb_db_context *ctdb_db;
  	TDB_DATA key;
  	TDB_DATA *data;
diff --cc ctdb/lib/talloc/talloc.h
index 75c130a2756,75c130a2756..bb068019cce
--- a/ctdb/lib/talloc/talloc.h
+++ b/ctdb/lib/talloc/talloc.h
@@@ -142,6 -142,6 +142,7 @@@ void talloc_report_depth_file(const voi
  void talloc_report_full(const void *ptr, FILE *f);
  void talloc_report(const void *ptr, FILE *f);
  void talloc_enable_null_tracking(void);
++void talloc_report_null_full(void);
  void talloc_disable_null_tracking(void);
  void talloc_enable_leak_report(void);
  void talloc_enable_leak_report_full(void);
diff --cc ctdb/tests/ctdb_fetch.c
index 91fff3825df,c5356edab32..e084b1bad8a
--- a/ctdb/tests/ctdb_fetch.c
+++ b/ctdb/tests/ctdb_fetch.c
@@@ -193,6 -197,8 +197,8 @@@ int main(int argc, const char *argv[]
  		}
  	}
  
 -	talloc_enable_leak_report_full();
++	/* talloc_enable_leak_report_full(); */
+ 
  	/* setup the remaining options for the main program to use */
  	extra_argv = poptGetArgs(pc);
  	if (extra_argv) {
diff --cc ctdb/tests/fetch.sh
index 157934caa0a,157934caa0a..9020fdc230e
--- a/ctdb/tests/fetch.sh
+++ b/ctdb/tests/fetch.sh
@@@ -3,15 -3,15 +3,15 @@@
  killall -q ctdb_fetch
  
  echo "Trying 2 nodes"
--bin/ctdb_fetch --nlist tests/nodes.txt --listen 127.0.0.2:9001 $* &
--bin/ctdb_fetch --nlist tests/nodes.txt --listen 127.0.0.1:9001 $* 
++$VALGRIND bin/ctdb_fetch --nlist tests/nodes.txt --listen 127.0.0.2:9001 $* &
++$VALGRIND bin/ctdb_fetch --nlist tests/nodes.txt --listen 127.0.0.1:9001 $* 
  
  killall -q ctdb_fetch
  
  echo "Trying 4 nodes"
--bin/ctdb_fetch --nlist tests/4nodes.txt --listen 127.0.0.4:9001 $* &
--bin/ctdb_fetch --nlist tests/4nodes.txt --listen 127.0.0.3:9001 $* &
--bin/ctdb_fetch --nlist tests/4nodes.txt --listen 127.0.0.2:9001 $* &
--bin/ctdb_fetch --nlist tests/4nodes.txt --listen 127.0.0.1:9001 $* 
++$VALGRIND bin/ctdb_fetch --nlist tests/4nodes.txt --listen 127.0.0.4:9001 $* &
++$VALGRIND bin/ctdb_fetch --nlist tests/4nodes.txt --listen 127.0.0.3:9001 $* &
++$VALGRIND bin/ctdb_fetch --nlist tests/4nodes.txt --listen 127.0.0.2:9001 $* &
++$VALGRIND bin/ctdb_fetch --nlist tests/4nodes.txt --listen 127.0.0.1:9001 $* 
  
  killall -q ctdb_fetch