[PATCH] ctdb-test: Add tests to verify queue processing
swen
swen at linux.ibm.com
Tue Mar 26 10:09:06 UTC 2019
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.
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
-------------- next part --------------
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190326/c477e7d8/signature.sig>
More information about the samba-technical
mailing list