> -----Original Message-----
> From: Olivier Matz [mailto:olivier.m...@6wind.com]
> Sent: Monday, February 25, 2019 4:59 AM
> To: Eads, Gage <gage.e...@intel.com>
> Cc: dev@dpdk.org; arybche...@solarflare.com; Richardson, Bruce
> <bruce.richard...@intel.com>; Ananyev, Konstantin
> <konstantin.anan...@intel.com>; gavin...@arm.com;
> honnappa.nagaraha...@arm.com; n...@arm.com; tho...@monjalon.net
> Subject: Re: [PATCH 3/7] test/stack: add stack test
>
> 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.e...@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.
>
Sure, I'll make popped_objs dynamically allocated.
> [...]
>
> > +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.
Ah, I didn't know rte_free() checks for NULL. Will remove.
>
> > +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.
Good catch. Will fix.
Thanks,
Gage
