-----Original Message-----
From: Olivier Matz [mailto:olivier.matz@6wind.com]
Sent: Monday, February 25, 2019 4:59 AM
To: Eads, Gage <redacted>
Cc: dev@dpdk.org; arybchenko@solarflare.com; Richardson, Bruce
[off-list ref]; Ananyev, Konstantin
[off-list ref]; gavin.hu@arm.com;
Honnappa.Nagarahalli@arm.com; nd@arm.com; thomas@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:

quoted

stack_autotest performs positive and negative testing of the stack
API, and exercises the push and pop datapath functions with all available

lcores.

quoted

Signed-off-by: Gage Eads <redacted>

[...]

quoted

--- /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.

[...]

quoted

+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.

quoted

+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