| Message ID | 1510236133-6926-1-git-send-email-steffan.karger@fox-it.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Hi, On 09/11/17 22:02, Steffan Karger wrote: > buffer_list_aggregate_separator() would merge buffer_list entries until it > had exceeded the provided max_len, instead of stopping *before* exceeding > the max value. > > Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> > --- > v2: rebase on 1/5 v2 (other patches should still apply) > > src/openvpn/buffer.c | 17 +++++++++++++---- > src/openvpn/buffer.h | 2 +- > tests/unit_tests/openvpn/test_buffer.c | 11 +++++++---- > 3 files changed, 21 insertions(+), 9 deletions(-) > > diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c > index 583c5c7..f72d525 100644 > --- a/src/openvpn/buffer.c > +++ b/src/openvpn/buffer.c > @@ -1231,7 +1231,8 @@ buffer_list_peek(struct buffer_list *ol) > } > > void > -buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const char *sep) > +buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len, > + const char *sep) > { > int sep_len = strlen(sep); > > @@ -1240,10 +1241,18 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const > struct buffer_entry *more = bl->head; > size_t size = 0; > int count = 0; > - for (count = 0; more && size <= max; ++count) > + for (count = 0; more; ++count) > { > - size += BLEN(&more->buf) + sep_len; > - more = more->next; > + size_t extra_len = BLEN(&more->buf) + sep_len; > + if (size + extra_len <= max_len) > + { > + size += extra_len; > + more = more->next; > + } > + else > + { > + break; > + } The code looks correct as it simply changes the looping condition and moves it inside the loop to stay below the 80chars. However, my taste would suggest to change the inner if-else block as follows: if (condition) { break; } rest; simply to avoid non-needed indentation and follow the usual "this is bad, we can't continue, thus go away" logic. But again, this is just my taste. Other maintainers can decide what is better. > } > > if (count >= 2) > diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h > index 1ed5631..499c75e 100644 > --- a/src/openvpn/buffer.h > +++ b/src/openvpn/buffer.h > @@ -1102,7 +1102,7 @@ void buffer_list_pop(struct buffer_list *ol); > > void buffer_list_aggregate(struct buffer_list *bl, const size_t max); > > -void buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const char *sep); > +void buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len, const char *sep); why not going on a new line now that you are touching it? (I know it exceeded 80 chars even before your change) > > struct buffer_list *buffer_list_file(const char *fn, int max_line_len); > > diff --git a/tests/unit_tests/openvpn/test_buffer.c b/tests/unit_tests/openvpn/test_buffer.c > index f5830a1..99e6530 100644 > --- a/tests/unit_tests/openvpn/test_buffer.c > +++ b/tests/unit_tests/openvpn/test_buffer.c > @@ -135,13 +135,16 @@ static void > test_buffer_list_aggregate_separator_two(void **state) > { > struct test_buffer_list_aggregate_ctx *ctx = *state; > + const char *expected = teststr1 testsep teststr2 testsep; > > - /* Aggregate the first two elements */ > - /* FIXME this exceeds the supplied max */ > - buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep); > + /* Aggregate the first two elements > + * (add 1 to max_len to test if "three" is not sneaked in too) > + */ > + buffer_list_aggregate_separator(ctx->one_two_three, strlen(expected)+1, > + testsep); Like in the previous patch, the '+' should be surrounded by spaces. > assert_int_equal(ctx->one_two_three->size, 2); > struct buffer *buf = buffer_list_peek(ctx->one_two_three); > - assert_buf_equals_str(buf, teststr1 testsep teststr2 testsep); > + assert_buf_equals_str(buf, expected); > } > > static void > Another FIXME thrown away :-) For the code correctness: Acked-by: Antonio Quartulli <a@unstable.cc> Cheers,
diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index 583c5c7..f72d525 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -1231,7 +1231,8 @@ buffer_list_peek(struct buffer_list *ol) } void -buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const char *sep) +buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len, + const char *sep) { int sep_len = strlen(sep); @@ -1240,10 +1241,18 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const struct buffer_entry *more = bl->head; size_t size = 0; int count = 0; - for (count = 0; more && size <= max; ++count) + for (count = 0; more; ++count) { - size += BLEN(&more->buf) + sep_len; - more = more->next; + size_t extra_len = BLEN(&more->buf) + sep_len; + if (size + extra_len <= max_len) + { + size += extra_len; + more = more->next; + } + else + { + break; + } } if (count >= 2) diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h index 1ed5631..499c75e 100644 --- a/src/openvpn/buffer.h +++ b/src/openvpn/buffer.h @@ -1102,7 +1102,7 @@ void buffer_list_pop(struct buffer_list *ol); void buffer_list_aggregate(struct buffer_list *bl, const size_t max); -void buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const char *sep); +void buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len, const char *sep); struct buffer_list *buffer_list_file(const char *fn, int max_line_len); diff --git a/tests/unit_tests/openvpn/test_buffer.c b/tests/unit_tests/openvpn/test_buffer.c index f5830a1..99e6530 100644 --- a/tests/unit_tests/openvpn/test_buffer.c +++ b/tests/unit_tests/openvpn/test_buffer.c @@ -135,13 +135,16 @@ static void test_buffer_list_aggregate_separator_two(void **state) { struct test_buffer_list_aggregate_ctx *ctx = *state; + const char *expected = teststr1 testsep teststr2 testsep; - /* Aggregate the first two elements */ - /* FIXME this exceeds the supplied max */ - buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep); + /* Aggregate the first two elements + * (add 1 to max_len to test if "three" is not sneaked in too) + */ + buffer_list_aggregate_separator(ctx->one_two_three, strlen(expected)+1, + testsep); assert_int_equal(ctx->one_two_three->size, 2); struct buffer *buf = buffer_list_peek(ctx->one_two_three); - assert_buf_equals_str(buf, teststr1 testsep teststr2 testsep); + assert_buf_equals_str(buf, expected); } static void
buffer_list_aggregate_separator() would merge buffer_list entries until it had exceeded the provided max_len, instead of stopping *before* exceeding the max value. Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> --- v2: rebase on 1/5 v2 (other patches should still apply) src/openvpn/buffer.c | 17 +++++++++++++---- src/openvpn/buffer.h | 2 +- tests/unit_tests/openvpn/test_buffer.c | 11 +++++++---- 3 files changed, 21 insertions(+), 9 deletions(-)