[1/3,net-next,RFC] selftest: add abstractions for net selftests

Submitted by Alexandru Copot on April 9, 2013, 10:30 a.m.

Details

Message ID 1365503461-26309-2-git-send-email-alex.mihai.c@gmail.com
State RFC
Delegated to: David Miller
Headers show

Commit Message

Alexandru Copot April 9, 2013, 10:30 a.m.
Signed-of by Alexandru Copot <alex.mihai.c@gmail.com>
Cc: Daniel Baluta <dbaluta@ixiacom.com>
---
 tools/testing/selftests/net/selftests.c | 30 +++++++++++++++++++++++
 tools/testing/selftests/net/selftests.h | 42 +++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)
 create mode 100644 tools/testing/selftests/net/selftests.c
 create mode 100644 tools/testing/selftests/net/selftests.h

Comments

Daniel Borkmann April 9, 2013, 11:13 a.m.
On 04/09/2013 12:30 PM, Alexandru Copot wrote:
> Signed-of by Alexandru Copot <alex.mihai.c@gmail.com>
> Cc: Daniel Baluta <dbaluta@ixiacom.com>
> ---
>   tools/testing/selftests/net/selftests.c | 30 +++++++++++++++++++++++
>   tools/testing/selftests/net/selftests.h | 42 +++++++++++++++++++++++++++++++++
>   2 files changed, 72 insertions(+)
>   create mode 100644 tools/testing/selftests/net/selftests.c
>   create mode 100644 tools/testing/selftests/net/selftests.h
>
> diff --git a/tools/testing/selftests/net/selftests.c b/tools/testing/selftests/net/selftests.c
> new file mode 100644
> index 0000000..cd6e427
> --- /dev/null
> +++ b/tools/testing/selftests/net/selftests.c
> @@ -0,0 +1,30 @@
> +#include <stdio.h>
> +
> +#include "selftests.h"
> +
> +int run_all_tests(struct generic_test *test, void *param)
> +{
> +	int i;
> +	int rc, allrc;
> +	char *ptr;
> +
> +	rc = test->prepare ? test->prepare(param) : 0;
> +	if (rc)
> +		return rc;
> +
> +	allrc = 0;

Nitpick: this could already have been initialized above.

> +	printf("Testing: %s ", test->name);
> +	ptr = test->testcases;

ditto

> +	for (i = 0; i < test->testcase_count; i++) {
> +		rc = test->run(ptr);
> +		allrc |= rc;
> +
> +		if (test->abort_on_fail && rc) {
> +			printf("Testcase %d failed, aborting\n", i);
> +		}

I think here you wanted to abort but didn't?

> +
> +		ptr += test->testcase_size;
> +	}
> +	printf("\t\t%s\n", allrc ? "[FAIL]" : "[PASS]");
> +	return allrc;
> +}
> diff --git a/tools/testing/selftests/net/selftests.h b/tools/testing/selftests/net/selftests.h
> new file mode 100644
> index 0000000..e289f03
> --- /dev/null
> +++ b/tools/testing/selftests/net/selftests.h
> @@ -0,0 +1,42 @@
> +#ifndef SELFTESTS_H
> +#define SELFTESTS_H
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +
> +#define ASSERT(cond) do {								\
> +	if (!(cond))     {								\
> +			fprintf(stderr, "%s:%d %s\n", __FILE__, __LINE__, #cond);	\
> +			perror("");							\
> +			exit(EXIT_FAILURE);						\
> +		}									\
> +} while (0)
> +
> +#define CHECK(cond,fmt,...)			        \
> +	do {						\
> +		if (!(cond)) {                          \
> +			fprintf(stderr, "(%s, %d): " fmt,	\
> +					__FILE__, __LINE__, ##__VA_ARGS__); \
> +			perror("");              	\
> +			return 1;			\
> +		}					\
> +	} while (0)

Isn't it a bit error-prone if in the middle of somewhere this check fails
and the function suddenly returns 1?

What if this is called from a function that was declared as void or to
return a pointer to a struct etc.?

> +struct generic_test {
> +	const char *name;
> +	void *testcases;
> +	int testcase_size;
> +	int testcase_count;
> +
> +	int abort_on_fail;
> +
> +	int (*prepare)(void *);
> +	int (*run)(void *);
> +	int (*cleanup)(void *);
> +};
> +
> +int run_all_tests(struct generic_test *test, void *param);
> +
> +#endif
> +
>

Nitpick: here's an extra newline at the end of the file.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandru Copot April 9, 2013, 11:24 a.m.
On Tue, Apr 9, 2013 at 2:13 PM, Daniel Borkmann <dborkman@redhat.com> wrote:
>> +       for (i = 0; i < test->testcase_count; i++) {
>> +               rc = test->run(ptr);
>> +               allrc |= rc;
>> +
>> +               if (test->abort_on_fail && rc) {
>> +                       printf("Testcase %d failed, aborting\n", i);
>> +               }
>
>
> I think here you wanted to abort but didn't?

Yes, I forgot to break;

>> +#define CHECK(cond,fmt,...)                            \
>> +       do {                                            \
>> +               if (!(cond)) {                          \
>> +                       fprintf(stderr, "(%s, %d): " fmt,       \
>> +                                       __FILE__, __LINE__,
>> ##__VA_ARGS__); \
>> +                       perror("");                     \
>> +                       return 1;                       \
>> +               }                                       \
>> +       } while (0)
>
>
> Isn't it a bit error-prone if in the middle of somewhere this check fails
> and the function suddenly returns 1?
>
> What if this is called from a function that was declared as void or to
> return a pointer to a struct etc.?

Well, I tought of using this only in your high-level testcase methods
(test->run()).
It is also easier to see what is actually being tested.

For anything else the user is free to use any other functions or
return conventions
as the test requires.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Borkmann April 9, 2013, 11:32 a.m.
On 04/09/2013 01:24 PM, Alexandru Copot wrote:
> On Tue, Apr 9, 2013 at 2:13 PM, Daniel Borkmann <dborkman@redhat.com> wrote:

>>> +#define CHECK(cond,fmt,...)                            \
>>> +       do {                                            \
>>> +               if (!(cond)) {                          \
>>> +                       fprintf(stderr, "(%s, %d): " fmt,       \
>>> +                                       __FILE__, __LINE__,
>>> ##__VA_ARGS__); \
>>> +                       perror("");                     \
>>> +                       return 1;                       \
>>> +               }                                       \
>>> +       } while (0)
>>
>>
>> Isn't it a bit error-prone if in the middle of somewhere this check fails
>> and the function suddenly returns 1?
>>
>> What if this is called from a function that was declared as void or to
>> return a pointer to a struct etc.?
>
> Well, I tought of using this only in your high-level testcase methods
> (test->run()).
> It is also easier to see what is actually being tested.
>
> For anything else the user is free to use any other functions or
> return conventions
> as the test requires.

Hm, then, still not convinced about the CHECK macro. In worst case this at
least needs a comment, so that people will not misuse that, but with your
two statements above, it seems likely that people could also start using it
in "any other functions or return conventions as the test requires". ;-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov April 9, 2013, 1:39 p.m.
Hello.

On 09-04-2013 14:30, Alexandru Copot wrote:

> Signed-of by Alexandru Copot <alex.mihai.c@gmail.com>
> Cc: Daniel Baluta <dbaluta@ixiacom.com>
[...]

> diff --git a/tools/testing/selftests/net/selftests.c b/tools/testing/selftests/net/selftests.c
> new file mode 100644
> index 0000000..cd6e427
> --- /dev/null
> +++ b/tools/testing/selftests/net/selftests.c
> @@ -0,0 +1,30 @@
> +#include <stdio.h>
> +
> +#include "selftests.h"
> +
> +int run_all_tests(struct generic_test *test, void *param)
> +{
> +	int i;
> +	int rc, allrc;
> +	char *ptr;
> +
> +	rc = test->prepare ? test->prepare(param) : 0;
> +	if (rc)
> +		return rc;
> +
> +	allrc = 0;
> +	printf("Testing: %s ", test->name);
> +	ptr = test->testcases;
> +	for (i = 0; i < test->testcase_count; i++) {
> +		rc = test->run(ptr);
> +		allrc |= rc;
> +
> +		if (test->abort_on_fail && rc) {
> +			printf("Testcase %d failed, aborting\n", i);
> +		}

    Nit: {} not needed here, at least if you folow the Linux coding style (you 
seem to).

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Borkmann April 9, 2013, 1:54 p.m.
On 04/09/2013 03:39 PM, Sergei Shtylyov wrote:
> On 09-04-2013 14:30, Alexandru Copot wrote:
>> Signed-of by Alexandru Copot <alex.mihai.c@gmail.com>
>> Cc: Daniel Baluta <dbaluta@ixiacom.com>
> [...]
>
>> diff --git a/tools/testing/selftests/net/selftests.c b/tools/testing/selftests/net/selftests.c
>> new file mode 100644
>> index 0000000..cd6e427
>> --- /dev/null
>> +++ b/tools/testing/selftests/net/selftests.c
>> @@ -0,0 +1,30 @@
[...]
>> +    for (i = 0; i < test->testcase_count; i++) {
>> +        rc = test->run(ptr);
>> +        allrc |= rc;
>> +
>> +        if (test->abort_on_fail && rc) {
>> +            printf("Testcase %d failed, aborting\n", i);
>> +        }
>
>     Nit: {} not needed here, at least if you folow the Linux coding style (you seem to).

We already figured out earlier in this thread that a ``break'' was missing. ;-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/tools/testing/selftests/net/selftests.c b/tools/testing/selftests/net/selftests.c
new file mode 100644
index 0000000..cd6e427
--- /dev/null
+++ b/tools/testing/selftests/net/selftests.c
@@ -0,0 +1,30 @@ 
+#include <stdio.h>
+
+#include "selftests.h"
+
+int run_all_tests(struct generic_test *test, void *param)
+{
+	int i;
+	int rc, allrc;
+	char *ptr;
+
+	rc = test->prepare ? test->prepare(param) : 0;
+	if (rc)
+		return rc;
+
+	allrc = 0;
+	printf("Testing: %s ", test->name);
+	ptr = test->testcases;
+	for (i = 0; i < test->testcase_count; i++) {
+		rc = test->run(ptr);
+		allrc |= rc;
+
+		if (test->abort_on_fail && rc) {
+			printf("Testcase %d failed, aborting\n", i);
+		}
+
+		ptr += test->testcase_size;
+	}
+	printf("\t\t%s\n", allrc ? "[FAIL]" : "[PASS]");
+	return allrc;
+}
diff --git a/tools/testing/selftests/net/selftests.h b/tools/testing/selftests/net/selftests.h
new file mode 100644
index 0000000..e289f03
--- /dev/null
+++ b/tools/testing/selftests/net/selftests.h
@@ -0,0 +1,42 @@ 
+#ifndef SELFTESTS_H
+#define SELFTESTS_H
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
+
+#define ASSERT(cond) do {								\
+	if (!(cond))     {								\
+			fprintf(stderr, "%s:%d %s\n", __FILE__, __LINE__, #cond);	\
+			perror("");							\
+			exit(EXIT_FAILURE);						\
+		}									\
+} while (0)
+
+#define CHECK(cond,fmt,...)			        \
+	do {						\
+		if (!(cond)) {                          \
+			fprintf(stderr, "(%s, %d): " fmt,	\
+					__FILE__, __LINE__, ##__VA_ARGS__); \
+			perror("");              	\
+			return 1;			\
+		}					\
+	} while (0)
+
+struct generic_test {
+	const char *name;
+	void *testcases;
+	int testcase_size;
+	int testcase_count;
+
+	int abort_on_fail;
+
+	int (*prepare)(void *);
+	int (*run)(void *);
+	int (*cleanup)(void *);
+};
+
+int run_all_tests(struct generic_test *test, void *param);
+
+#endif
+