[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