[PATCH] ctdb-test: Add tests to verify queue processing

Christof Schmitt cs at samba.org
Tue Apr 9 23:11:10 UTC 2019


On Tue, Mar 26, 2019 at 11:09:06AM +0100, swen via samba-technical wrote:
> Hi Martin
> 
> Thanks for your reply.... I updated the patch-set as suggested and
> fixed the typo.
> Added you Reviewed-by to the messages as well.
> 
> Another, 2nd team-reviewer please.
> 
> Thanks for your support in advvance.

Looks good, RB+

Pushed to autobuild.

Christof

> 
> Cheers Swen
> On Tue, 2019-03-26 at 20:43 +1100, Martin Schwenke wrote:
> > On Thu, 21 Mar 2019 11:05:29 +0100, swen <swen at linux.ibm.com> wrote:
> > 
> > > On Thu, 2019-03-21 at 17:15 +1100, Martin Schwenke wrote:
> > > > On Mon, 18 Mar 2019 15:34:30 +0100, swen <swen at linux.ibm.com>
> > > > wrote:
> > > >   
> > > > > On Mon, 2019-03-18 at 21:58 +1100, Martin Schwenke wrote:  
> > >  [...]  
> > > > > Thanks for your reply.
> > > > > I integrated the patches into ctdb_io_test.c and performed all
> > > > > the
> > > > > other updates as well (ctdb_io_test_001.sh)
> > > > > 
> > > > > Please review and push if happy.  
> > > > 
> > > > I would suggest not changing test_setup() to *return* the queue,
> > > > while
> > > > dropping the ctdb output parameter.  That forces the queue it
> > > > into
> > > > tests
> > > > that don't use it.
> > > > 
> > > > Instead, as I suggested in my previous email, I suggest adding an
> > > > optional output argument for the queue and only pass it out if
> > > > non-
> > > > NULL
> > > > is passed in.  That way, the change to existing tests is minimal:
> > > > just
> > > > pass NULL for the extra argument.  This makes it clear that those
> > > > tests
> > > > aren't looking inside the queue.
> > > > 
> > > > The new tests could then follow the existing pattern of using
> > > > ctdb-  
> > > > > ev  
> > > > for the event loop and using queue when necessary.
> > > > 
> > > > How does that sound?  
> > > Well, I would have preferred to "use" ctdb_io as it is by the real
> > > functional consumers in the code, meaning, to have the queue as the
> > > center of gravity.
> > > But I do see your point.
> > > 
> > > Therefore, I changed it to the way you prefer.
> > > 
> > > Please review and push if happy 
> > > ... you shoudld be totally excited now :-) 
> > 
> > Looks good.  Nice tests...
> > 
> > I'd probably suggest merging the enabling of the tests, currently in
> > patch 4, into patches 2 & 3.  There's nothing terribly exciting
> > happening in patch 4 and enabling the test in the script fits
> > into the 1 thing that the patches 2 & 3 are doing: adding a test.
> > However, I won't spend time arguing about it.  You could repost with
> > that change or not bother.  :-)
> > 
> > Depending on whether this happens, there's a minor typo in the commit
> > message of patch 3: s/poacket/packet/ .  If you repost without patch
> > 4 as above then please fix it, otherwise the 2nd reviewer can fix
> > it.  When they add the Reviewed-by: tags.
> > 
> > Reviewed-by: Martin Schwenke <martin at meltin.net>
> > 
> > 2nd team reviewer, please?
> > 
> > Thanks...
> > 
> > > P.S.: One thing I have to say.... I really, really dislike the
> > > hungarian notation, totally odd stuff, and I felt physical pain to
> > > not
> > > change it and even worse to add to it.
> > 
> > I had to look up "Hungarian notation".  Thanks for not changing it.
> > Christof obviously likes it...  ;-)
> > 
> > peace & happiness,
> > martin

> From 87d5f6a3566b0dc900638811517ab06d5807292a Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at linux.ibm.com>
> Date: Mon, 18 Mar 2019 15:15:25 +0100
> Subject: [PATCH 1/3] ctdb-test: Modify ctdb_io_test test_setup to provide
>  queue reference
> 
> Some test scenarios require access to the created queue.
> Prepare the test_setup function to provide it as additional parameter.
> 
> Signed-off-by: Swen Schillig <swen at linux.ibm.com>
> Reviewed-by: Martin Schwenke <martin at meltin.net>
> ---
>  ctdb/tests/src/ctdb_io_test.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/ctdb/tests/src/ctdb_io_test.c b/ctdb/tests/src/ctdb_io_test.c
> index 5a2f81538a1..668b1f0afe5 100644
> --- a/ctdb/tests/src/ctdb_io_test.c
> +++ b/ctdb/tests/src/ctdb_io_test.c
> @@ -34,7 +34,8 @@ void ctdb_set_error(struct ctdb_context *ctdb, const char *fmt, ...)
>  
>  static void test_setup(ctdb_queue_cb_fn_t cb,
>  		       int *pfd,
> -		       struct ctdb_context **pctdb)
> +		       struct ctdb_context **pctdb,
> +		       struct ctdb_queue **pqueue)
>  {
>  	int pipefd[2], ret;
>  	struct ctdb_context *ctdb;
> @@ -54,6 +55,9 @@ static void test_setup(ctdb_queue_cb_fn_t cb,
>  
>  	*pctdb = ctdb;
>  	*pfd = pipefd[1];
> +	if (pqueue != NULL) {
> +		*pqueue = queue;
> +	}
>  }
>  
>  static const size_t test1_req_len = 8;
> @@ -76,7 +80,7 @@ static void test1(void)
>  	int fd, ret;
>  	uint32_t pkt_size;
>  
> -	test_setup(test1_callback, &fd, &ctdb);
> +	test_setup(test1_callback, &fd, &ctdb, NULL);
>  
>  	pkt_size = sizeof(uint32_t) + test1_req_len;
>  	ret = write(fd, &pkt_size, sizeof(pkt_size));
> @@ -116,7 +120,7 @@ static void test2(void)
>  		req[i] = i % CHAR_MAX;
>  	}
>  
> -	test_setup(test2_callback, &fd, &ctdb);
> +	test_setup(test2_callback, &fd, &ctdb, NULL);
>  
>  	/*
>  	 * request 0
> -- 
> 2.20.1
> 
> 
> From 0eefb3fa37e8bede1f2f0f1399cea7f0b29f982f Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at linux.ibm.com>
> Date: Mon, 18 Mar 2019 15:22:19 +0100
> Subject: [PATCH 2/3] ctdb-test: Adding test case verifying data in buffer move
> 
> Signed-off-by: Swen Schillig <swen at linux.ibm.com>
> Reviewed-by: Martin Schwenke <martin at meltin.net>
> ---
>  ctdb/tests/cunit/ctdb_io_test_001.sh |  1 +
>  ctdb/tests/src/ctdb_io_test.c        | 76 ++++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+)
> 
> diff --git a/ctdb/tests/cunit/ctdb_io_test_001.sh b/ctdb/tests/cunit/ctdb_io_test_001.sh
> index 15842ea8e17..6851d524bf5 100755
> --- a/ctdb/tests/cunit/ctdb_io_test_001.sh
> +++ b/ctdb/tests/cunit/ctdb_io_test_001.sh
> @@ -6,3 +6,4 @@ ok_null
>  
>  unit_test ctdb_io_test 1
>  unit_test ctdb_io_test 2
> +unit_test ctdb_io_test 3
> diff --git a/ctdb/tests/src/ctdb_io_test.c b/ctdb/tests/src/ctdb_io_test.c
> index 668b1f0afe5..ec1d25859f0 100644
> --- a/ctdb/tests/src/ctdb_io_test.c
> +++ b/ctdb/tests/src/ctdb_io_test.c
> @@ -172,6 +172,78 @@ static void test2(void)
>  	TALLOC_FREE(ctdb);
>  }
>  
> +static void test_cb(uint8_t *data, size_t length, void *private_data)
> +{
> +	/* dummy handler, not verifying anything */
> +	TALLOC_FREE(data);
> +}
> +
> +static void test3(void)
> +{
> +	struct ctdb_context *ctdb;
> +	struct ctdb_queue *queue;
> +	uint32_t pkt_size;
> +	char *request;
> +	size_t req_len;
> +	int fd;
> +	int ret;
> +
> +	test_setup(test_cb, &fd, &ctdb, &queue);
> +	request = talloc_zero_size(queue, queue->buffer_size);
> +
> +	/*
> +	 * calculate a request length which will fit into the buffer
> +	 * but not twice. Because we need to write the size integer
> +	 * as well (4-bytes) we're guaranteed that no 2 packets will fit.
> +	 */
> +	req_len = queue->buffer_size >> 1;
> +
> +	/* writing first packet */
> +	pkt_size = sizeof(uint32_t) + req_len;
> +
> +	ret = write(fd, &pkt_size, sizeof(pkt_size));
> +	assert(ret == sizeof(pkt_size));
> +
> +	ret = write(fd, request, req_len);
> +	assert(ret == req_len);
> +
> +	/* writing second, incomplete packet */
> +	pkt_size = sizeof(uint32_t) + req_len;
> +
> +	ret = write(fd, &pkt_size, sizeof(pkt_size));
> +	assert(ret == sizeof(pkt_size));
> +
> +	ret = write(fd, request, req_len >> 1);
> +	assert(ret == req_len >> 1);
> +
> +	/* process...only 1st packet can be processed */
> +	tevent_loop_once(ctdb->ev);
> +
> +	/* we should see a progressed offset of req_len + sizeof(pkt_size) */
> +	assert(queue->buffer.offset == req_len + sizeof(pkt_size));
> +
> +	/* writing another few bytes of the still incomplete packet */
> +	ret = write(fd, request, (req_len >> 1) - 1);
> +	assert(ret == (req_len >> 1) - 1);
> +
> +	/*
> +	 * the packet is still incomplete and connot be processed
> +	 * but the packet data had to be moved in the buffer in order
> +	 * to fetch the new 199 bytes -> offset must be 0 now.
> +	 */
> +	tevent_loop_once(ctdb->ev);
> +	/*
> +	 * needs to be called twice as an incomplete packet
> +	 * does not trigger a schedule_immediate
> +	 */
> +	tevent_loop_once(ctdb->ev);
> +
> +	assert(queue->buffer.offset == 0);
> +
> +	TALLOC_FREE(ctdb);
> +}
> +
> +
>  int main(int argc, const char **argv)
>  {
>  	int num;
> @@ -192,6 +264,10 @@ int main(int argc, const char **argv)
>  		test2();
>  		break;
>  
> +	case 3:
> +		test3();
> +		break;
> +
>  	default:
>  		fprintf(stderr, "Unknown test number %s\n", argv[1]);
>  	}
> -- 
> 2.20.1
> 
> 
> From 1e5355f39de7ef77c9812e3435edd2b8431d4f90 Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at linux.ibm.com>
> Date: Mon, 18 Mar 2019 15:25:54 +0100
> Subject: [PATCH 3/3] ctdb-test: Adding test case to verify queue resizeing
> 
> If a data packet arrives which exceeds the queue's current buffer size,
> the buffer needs to be increased to hold the full packet. Once the packet
> is processed the buffer size should be decreased to its standard size again.
> This test case verifies this process.
> 
> Signed-off-by: Swen Schillig <swen at linux.ibm.com>
> Reviewed-by: Martin Schwenke <martin at meltin.net>
> ---
>  ctdb/tests/cunit/ctdb_io_test_001.sh |  1 +
>  ctdb/tests/src/ctdb_io_test.c        | 74 ++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/ctdb/tests/cunit/ctdb_io_test_001.sh b/ctdb/tests/cunit/ctdb_io_test_001.sh
> index 6851d524bf5..b6d3bce4782 100755
> --- a/ctdb/tests/cunit/ctdb_io_test_001.sh
> +++ b/ctdb/tests/cunit/ctdb_io_test_001.sh
> @@ -7,3 +7,4 @@ ok_null
>  unit_test ctdb_io_test 1
>  unit_test ctdb_io_test 2
>  unit_test ctdb_io_test 3
> +unit_test ctdb_io_test 4
> diff --git a/ctdb/tests/src/ctdb_io_test.c b/ctdb/tests/src/ctdb_io_test.c
> index ec1d25859f0..9cd02aa0eaa 100644
> --- a/ctdb/tests/src/ctdb_io_test.c
> +++ b/ctdb/tests/src/ctdb_io_test.c
> @@ -243,6 +243,76 @@ static void test3(void)
>  	TALLOC_FREE(ctdb);
>  }
>  
> +static void test4(void)
> +{
> +	struct ctdb_context *ctdb;
> +	struct ctdb_queue *queue;
> +	uint32_t pkt_size;
> +	char *request;
> +	size_t req_len;
> +	int fd;
> +	int ret;
> +
> +	test_setup(test_cb, &fd, &ctdb, &queue);
> +
> +	req_len = queue->buffer_size << 1; /* double the buffer size */
> +	request = talloc_zero_size(queue, req_len);
> +
> +	/* writing first part of packet exceeding standard buffer size */
> +	pkt_size = sizeof(uint32_t) + req_len;
> +
> +	ret = write(fd, &pkt_size, sizeof(pkt_size));
> +	assert(ret == sizeof(pkt_size));
> +
> +	ret = write(fd, request, req_len - (queue->buffer_size >> 1));
> +	assert(ret == req_len - (queue->buffer_size >> 1));
> +
> +	/*
> +	 * process...
> +	 * this needs to be done to have things changed
> +	 */
> +	tevent_loop_once(ctdb->ev);
> +	/*
> +	 * needs to be called twice as an initial incomplete packet
> +	 * does not trigger a schedule_immediate
> +	 */
> +	tevent_loop_once(ctdb->ev);
> +
> +	/* the buffer should be resized to packet size now */
> +	assert(queue->buffer.size == pkt_size);
> +
> +	/* writing remaining data */
> +	ret = write(fd, request, queue->buffer_size >> 1);
> +	assert(ret == (queue->buffer_size >> 1));
> +
> +	/* process... */
> +	tevent_loop_once(ctdb->ev);
> +
> +	/*
> +	 * the buffer was increased beyond its standard size.
> +	 * once packet got processed, the buffer has to be free'd
> +	 * and will be re-allocated with standard size on new request arrival.
> +	 */
> +
> +	assert(queue->buffer.size == 0);
> +
> +	/* writing new packet to verify standard buffer size */
> +	pkt_size = sizeof(uint32_t) + (queue->buffer_size >> 1);
> +
> +	ret = write(fd, &pkt_size, sizeof(pkt_size));
> +	assert(ret == sizeof(pkt_size));
> +
> +	ret = write(fd, request, (queue->buffer_size >> 1));
> +	assert(ret == (queue->buffer_size >> 1));
> +
> +	/* process... */
> +	tevent_loop_once(ctdb->ev);
> +
> +	/* back to standard buffer size */
> +	assert(queue->buffer.size == queue->buffer_size);
> +
> +	TALLOC_FREE(ctdb);
> +}
>  
>  int main(int argc, const char **argv)
>  {
> @@ -268,6 +338,10 @@ int main(int argc, const char **argv)
>  		test3();
>  		break;
>  
> +	case 4:
> +		test4();
> +		break;
> +
>  	default:
>  		fprintf(stderr, "Unknown test number %s\n", argv[1]);
>  	}
> -- 
> 2.20.1
> 






More information about the samba-technical mailing list