Re: [dpdk-dev] [PATCH 3/7] test/stack: add stack test

[Olivier Matz <olivier.matz@6wind.com>]

On Fri, Feb 22, 2019 at 10:06:51AM -0600, Gage Eads wrote:
> stack_autotest performs positive and negative testing of the stack API, and
> exercises the push and pop datapath functions with all available lcores.
> 
> Signed-off-by: Gage Eads <gage.eads@intel.com>

[...]

> --- /dev/null
> +++ b/test/test/test_stack.c
> @@ -0,0 +1,394 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2019 Intel Corporation
> + */
> +
> +#include <string.h>
> +
> +#include <rte_lcore.h>
> +#include <rte_malloc.h>
> +#include <rte_random.h>
> +#include <rte_stack.h>
> +
> +#include "test.h"
> +
> +#define STACK_SIZE 4096
> +#define MAX_BULK 32
> +
> +static int
> +test_stack_push_pop(struct rte_stack *s, void **obj_table, unsigned int bulk_sz)
> +{
> +	void *popped_objs[STACK_SIZE];
> +	unsigned int i, ret;

Here, a dynamic sized table is used. In test_stack_basic() below, it
uses a heap-based allocation for the same purpose. I think it would be
more consistent to have the same method for both. I suggest to allocate
in heap to avoid a stack overflow if STACK_SIZE is increased in the
future.

[...]

> +static int
> +test_stack_basic(void)
> +{
> +	struct rte_stack *s = NULL;
> +	void **obj_table = NULL;
> +	int i, ret = -1;
> +
> +	obj_table = rte_calloc(NULL, STACK_SIZE, sizeof(void *), 0);
> +	if (obj_table == NULL) {
> +		printf("[%s():%u] failed to calloc %lu bytes\n",
> +		       __func__, __LINE__, STACK_SIZE * sizeof(void *));
> +		goto fail_test;
> +	}
> +
> +	for (i = 0; i < STACK_SIZE; i++)
> +		obj_table[i] = (void *)(uintptr_t)i;
> +
> +	s = rte_stack_create(__func__, STACK_SIZE, rte_socket_id(), 0);
> +	if (s == NULL) {
> +		printf("[%s():%u] failed to create a stack\n",
> +		       __func__, __LINE__);
> +		goto fail_test;
> +	}
> +
> +	if (rte_stack_lookup(__func__) != s) {
> +		printf("[%s():%u] failed to lookup a stack\n",
> +		       __func__, __LINE__);
> +		goto fail_test;
> +	}
> +
> +	if (rte_stack_count(s) != 0) {
> +		printf("[%s():%u] stack count: %u (expected 0)\n",
> +		       __func__, __LINE__, rte_stack_count(s));
> +		goto fail_test;
> +	}
> +
> +	if (rte_stack_free_count(s) != STACK_SIZE) {
> +		printf("[%s():%u] stack free count: %u (expected %u)\n",
> +		       __func__, __LINE__, rte_stack_count(s), STACK_SIZE);
> +		goto fail_test;
> +	}
> +
> +	ret = test_stack_push_pop(s, obj_table, 1);
> +	if (ret) {
> +		printf("[%s():%u] Single object push/pop failed\n",
> +		       __func__, __LINE__);
> +		goto fail_test;
> +	}
> +
> +	ret = test_stack_push_pop(s, obj_table, MAX_BULK);
> +	if (ret) {
> +		printf("[%s():%u] Bulk object push/pop failed\n",
> +		       __func__, __LINE__);
> +		goto fail_test;
> +	}
> +
> +	ret = rte_stack_push(s, obj_table, 2 * STACK_SIZE);
> +	if (ret != 0) {
> +		printf("[%s():%u] Excess objects push succeeded\n",
> +		       __func__, __LINE__);
> +		goto fail_test;
> +	}
> +
> +	ret = rte_stack_pop(s, obj_table, 1);
> +	if (ret != 0) {
> +		printf("[%s():%u] Empty stack pop succeeded\n",
> +		       __func__, __LINE__);
> +		goto fail_test;
> +	}
> +
> +	ret = 0;
> +
> +fail_test:
> +	rte_stack_free(s);
> +
> +	if (obj_table != NULL)
> +		rte_free(obj_table);
> +

The if can be removed.

> +static int
> +test_stack_name_length(void)
> +{
> +	char name[RTE_STACK_NAMESIZE + 1];
> +	struct rte_stack *s;
> +
> +	memset(name, 's', sizeof(name));
> +
> +	s = rte_stack_create(name, STACK_SIZE, rte_socket_id(), 0);
> +	if (s != NULL) {
> +		printf("[%s():%u] Failed to prevent long name\n",
> +		       __func__, __LINE__);
> +		return -1;
> +	}

Here, "name" is not a valid string (no \0 at the end). It does not hurt because
the length check is properly done in the lib, but we could imagine that the
wrong name is logged by the library on error, which would trigger a crash
here. So I suggest to pass a valid string instead.