[PATCH 0/3 v3] selftests: Add efivarfs tests

[PATCH 0/3 v3] selftests: Add efivarfs tests

[Jeremy Kerr]

Hi all,

The following patches add a small set of tests to the
tools/testing/selftests directory. These cover some of the basic
functionality of efivarfs.

Comments welcome - changes 2 and 3 cover some behaviour that Matt and I
have discussed, but I'd appreciate any wider comment on the semantics
covered by these.

Cheers,


Jeremy

--
v2: Remove qemu check, add a couple of tests
v3: Change expected empty read() behaviour to return EOF

---
Jeremy Kerr (3):
      selftests: Add tests for efivarfs
      selftests/efivarfs: Add empty file creation test
      selftests/efivarfs: Add create-read test

[PATCH 3/3 v3] selftests/efivarfs: Add create-read test

[Jeremy Kerr]

Test that reads from a newly-created efivarfs file (with no data
written) will return EOF.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

---
 tools/testing/selftests/efivarfs/Makefile      |    2 
 tools/testing/selftests/efivarfs/create-read.c |   38 +++++++++++++++++
 tools/testing/selftests/efivarfs/efivarfs.sh   |    7 +++
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/efivarfs/Makefile b/tools/testing/selftests/efivarfs/Makefile
index 1a943ee..b4a7aca 100644
--- a/tools/testing/selftests/efivarfs/Makefile
+++ b/tools/testing/selftests/efivarfs/Makefile
@@ -1,7 +1,7 @@
 CC = $(CROSS_COMPILE)gcc
 CFLAGS = -Wall
 
-test_objs = open-unlink
+test_objs = open-unlink create-read
 
 all: $(test_objs)
 
diff --git a/tools/testing/selftests/efivarfs/create-read.c b/tools/testing/selftests/efivarfs/create-read.c
new file mode 100644
index 0000000..7feef18
--- /dev/null
+++ b/tools/testing/selftests/efivarfs/create-read.c
@@ -0,0 +1,38 @@
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <string.h>
+
+int main(int argc, char **argv)
+{
+	const char *path;
+	char buf[4];
+	int fd, rc;
+
+	if (argc < 2) {
+		fprintf(stderr, "usage: %s <path>\n", argv[0]);
+		return EXIT_FAILURE;
+	}
+
+	path = argv[1];
+
+	/* create a test variable */
+	fd = open(path, O_RDWR | O_CREAT, 0600);
+	if (fd < 0) {
+		perror("open(O_WRONLY)");
+		return EXIT_FAILURE;
+	}
+
+	rc = read(fd, buf, sizeof(buf));
+	if (rc != 0) {
+		fprintf(stderr, "Reading a new var should return EOF\n");
+		return EXIT_FAILURE;
+	}
+
+	return EXIT_SUCCESS;
+}
diff --git a/tools/testing/selftests/efivarfs/efivarfs.sh b/tools/testing/selftests/efivarfs/efivarfs.sh
index 3af4b37..880cdd5 100755
--- a/tools/testing/selftests/efivarfs/efivarfs.sh
+++ b/tools/testing/selftests/efivarfs/efivarfs.sh
@@ -70,6 +70,12 @@ test_create_empty()
 	fi
 }
 
+test_create_read()
+{
+	local file=$efivarfs_mount/$FUNCNAME-$test_guid
+	./create-read $file
+}
+
 test_delete()
 {
 	local attrs='\x07\x00\x00\x00'
@@ -125,6 +131,7 @@ rc=0
 
 run_test test_create
 run_test test_create_empty
+run_test test_create_read
 run_test test_delete
 run_test test_zero_size_delete
 run_test test_open_unlink

[PATCH 2/3 v3] selftests/efivarfs: Add empty file creation test

[Jeremy Kerr]

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

---
 tools/testing/selftests/efivarfs/efivarfs.sh |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/testing/selftests/efivarfs/efivarfs.sh b/tools/testing/selftests/efivarfs/efivarfs.sh
index e8c0d27..3af4b37 100755
--- a/tools/testing/selftests/efivarfs/efivarfs.sh
+++ b/tools/testing/selftests/efivarfs/efivarfs.sh
@@ -58,6 +58,18 @@ test_create()
 	fi
 }
 
+test_create_empty()
+{
+	local file=$efivarfs_mount/$FUNCNAME-$test_guid
+
+	: > $file
+
+	if [ ! -e $file ]; then
+		echo "$file can not be created without writing" >&2
+		exit 1
+	fi
+}
+
 test_delete()
 {
 	local attrs='\x07\x00\x00\x00'
@@ -112,6 +124,7 @@ check_prereqs
 rc=0
 
 run_test test_create
+run_test test_create_empty
 run_test test_delete
 run_test test_zero_size_delete
 run_test test_open_unlink

[PATCH 1/3 v3] selftests: Add tests for efivarfs

[Jeremy Kerr]

This change adds a few initial efivarfs tests to the
tools/testing/selftests directory.

The open-unlink test is based on code from
Lingzhu Xiang <lxiang@redhat.com>.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

---
 tools/testing/selftests/Makefile               |    2 
 tools/testing/selftests/efivarfs/Makefile      |   12 +
 tools/testing/selftests/efivarfs/efivarfs.sh   |  119 +++++++++++++++++
 tools/testing/selftests/efivarfs/open-unlink.c |   63 +++++++++
 4 files changed, 195 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 85baf11..dee19dd 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -1,4 +1,4 @@
-TARGETS = breakpoints kcmp mqueue vm cpu-hotplug memory-hotplug
+TARGETS = breakpoints kcmp mqueue vm cpu-hotplug memory-hotplug efivarfs
 
 all:
 	for TARGET in $(TARGETS); do \
diff --git a/tools/testing/selftests/efivarfs/Makefile b/tools/testing/selftests/efivarfs/Makefile
new file mode 100644
index 0000000..1a943ee
--- /dev/null
+++ b/tools/testing/selftests/efivarfs/Makefile
@@ -0,0 +1,12 @@
+CC = $(CROSS_COMPILE)gcc
+CFLAGS = -Wall
+
+test_objs = open-unlink
+
+all: $(test_objs)
+
+run_tests: all
+	@./efivarfs.sh || echo "efivarfs selftests: [FAIL]"
+
+clean:
+	rm -f $(test_objs)
diff --git a/tools/testing/selftests/efivarfs/efivarfs.sh b/tools/testing/selftests/efivarfs/efivarfs.sh
new file mode 100755
index 0000000..e8c0d27
--- /dev/null
+++ b/tools/testing/selftests/efivarfs/efivarfs.sh
@@ -0,0 +1,119 @@
+#!/bin/bash
+
+efivarfs_mount=/sys/firmware/efi/efivars
+test_guid=210be57c-9849-4fc7-a635-e6382d1aec27
+
+check_prereqs()
+{
+	local msg="skip all tests:"
+
+	if [ $UID != 0 ]; then
+		echo $msg must be run as root >&2
+		exit 0
+	fi
+
+	if ! grep -q "^\S\+ $efivarfs_mount efivarfs" /proc/mounts; then
+		echo $msg efivarfs is not mounted on $efivarfs_mount >&2
+		exit 0
+	fi
+}
+
+run_test()
+{
+	local test="$1"
+
+	echo "--------------------"
+	echo "running $test"
+	echo "--------------------"
+
+	if [ "$(type -t $test)" = 'function' ]; then
+		( $test )
+	else
+		( ./$test )
+	fi
+
+	if [ $? -ne 0 ]; then
+		echo "  [FAIL]"
+		rc=1
+	else
+		echo "  [PASS]"
+	fi
+}
+
+test_create()
+{
+	local attrs='\x07\x00\x00\x00'
+	local file=$efivarfs_mount/$FUNCNAME-$test_guid
+
+	printf "$attrs\x00" > $file
+
+	if [ ! -e $file ]; then
+		echo "$file couldn't be created" >&2
+		exit 1
+	fi
+
+	if [ $(stat -c %s $file) -ne 5 ]; then
+		echo "$file has invalid size" >&2
+		exit 1
+	fi
+}
+
+test_delete()
+{
+	local attrs='\x07\x00\x00\x00'
+	local file=$efivarfs_mount/$FUNCNAME-$test_guid
+
+	printf "$attrs\x00" > $file
+
+	if [ ! -e $file ]; then
+		echo "$file couldn't be created" >&2
+		exit 1
+	fi
+
+	rm $file
+
+	if [ -e $file ]; then
+		echo "$file couldn't be deleted" >&2
+		exit 1
+	fi
+
+}
+
+# test that we can remove a variable by issuing a write with only
+# attributes specified
+test_zero_size_delete()
+{
+	local attrs='\x07\x00\x00\x00'
+	local file=$efivarfs_mount/$FUNCNAME-$test_guid
+
+	printf "$attrs\x00" > $file
+
+	if [ ! -e $file ]; then
+		echo "$file does not exist" >&2
+		exit 1
+	fi
+
+	printf "$attrs" > $file
+
+	if [ -e $file ]; then
+		echo "$file should have been deleted" >&2
+		exit 1
+	fi
+}
+
+test_open_unlink()
+{
+	local file=$efivarfs_mount/$FUNCNAME-$test_guid
+	./open-unlink $file
+}
+
+check_prereqs
+
+rc=0
+
+run_test test_create
+run_test test_delete
+run_test test_zero_size_delete
+run_test test_open_unlink
+
+exit $rc
diff --git a/tools/testing/selftests/efivarfs/open-unlink.c b/tools/testing/selftests/efivarfs/open-unlink.c
new file mode 100644
index 0000000..8c07644
--- /dev/null
+++ b/tools/testing/selftests/efivarfs/open-unlink.c
@@ -0,0 +1,63 @@
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+int main(int argc, char **argv)
+{
+	const char *path;
+	char buf[5];
+	int fd, rc;
+
+	if (argc < 2) {
+		fprintf(stderr, "usage: %s <path>\n", argv[0]);
+		return EXIT_FAILURE;
+	}
+
+	path = argv[1];
+
+	/* attributes: EFI_VARIABLE_NON_VOLATILE |
+	 *		EFI_VARIABLE_BOOTSERVICE_ACCESS |
+	 *		EFI_VARIABLE_RUNTIME_ACCESS
+	 */
+	*(uint32_t *)buf = 0x7;
+	buf[4] = 0;
+
+	/* create a test variable */
+	fd = open(path, O_WRONLY | O_CREAT);
+	if (fd < 0) {
+		perror("open(O_WRONLY)");
+		return EXIT_FAILURE;
+	}
+
+	rc = write(fd, buf, sizeof(buf));
+	if (rc != sizeof(buf)) {
+		perror("write");
+		return EXIT_FAILURE;
+	}
+
+	close(fd);
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0) {
+		perror("open");
+		return EXIT_FAILURE;
+	}
+
+	if (unlink(path) < 0) {
+		perror("unlink");
+		return EXIT_FAILURE;
+	}
+
+	rc = read(fd, buf, sizeof(buf));
+	if (rc > 0) {
+		fprintf(stderr, "reading from an unlinked variable "
+				"shouldn't be possible\n");
+		return EXIT_FAILURE;
+	}
+
+	return EXIT_SUCCESS;
+}

Re: [PATCH 1/3 v3] selftests: Add tests for efivarfs

[Andrew Morton]

On Wed, 06 Feb 2013 22:48:08 +0800
Jeremy Kerr <jk@ozlabs.org> wrote:

> This change adds a few initial efivarfs tests to the
> tools/testing/selftests directory.
> 
> The open-unlink test is based on code from
> Lingzhu Xiang <lxiang@redhat.com>.
> 
> ...
>
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -1,4 +1,4 @@
> -TARGETS = breakpoints kcmp mqueue vm cpu-hotplug memory-hotplug
> +TARGETS = breakpoints kcmp mqueue vm cpu-hotplug memory-hotplug efivarfs

bah.  This sort of Makefile construct is a wonderful source of patch
rejects and fixups.  I'll covert this to

--- a/tools/testing/selftests/Makefile~a
+++ a/tools/testing/selftests/Makefile
@@ -1,4 +1,11 @@
-TARGETS = breakpoints epoll kcmp mqueue vm cpu-hotplug memory-hotplug efivarfs
+TARGETS = breakpoints
+TARGETS += epoll
+TARGETS += kcmp
+TARGETS += mqueue
+TARGETS += vm
+TARGETS += cpu-hotplug
+TARGETS += memory-hotplug
+TARGETS += efivarfs
 
 all:
 	for TARGET in $(TARGETS); do \

> new file mode 100644
> index 0000000..1a943ee
> --- /dev/null
> +++ b/tools/testing/selftests/efivarfs/Makefile
> @@ -0,0 +1,12 @@
> +CC = $(CROSS_COMPILE)gcc
> +CFLAGS = -Wall
> +
> +test_objs = open-unlink
> +
> +all: $(test_objs)
> +
> +run_tests: all
> +	@./efivarfs.sh || echo "efivarfs selftests: [FAIL]"

Problem.  When I apply the patch, ./efivarfs.sh doesn't have execute
permissions.  I don't think there's a way of (reliably?) transporting
this with patch and diff.

So we should explicitly invoke sh or /bin/sh or $SHELL or whatever
here.  This problem is common to several Makefiles in tools/testing/selftests/

I'll do this for now:

--- a/tools/testing/selftests/efivarfs/Makefile~selftests-add-tests-for-efivarfs-fix
+++ a/tools/testing/selftests/efivarfs/Makefile
@@ -6,7 +6,7 @@ test_objs = open-unlink
 all: $(test_objs)
 
 run_tests: all
-	@./efivarfs.sh || echo "efivarfs selftests: [FAIL]"
+	@/bin/sh ./efivarfs.sh || echo "efivarfs selftests: [FAIL]"
 
 clean:
 	rm -f $(test_objs)

but I'm not sure I did it right :(


The general ruleset for selftests is: do as much as you can if you're not
root and don't take too long and don't break the build on any
architecture and don't cause the top-level "make run_tests" to fail if
your feature is unconfigured.

Does this code pass all that?

[PATCH] Documentation: Add a simple doc for selftests

[Jeremy Kerr]

On 08/02/13 07:13, Andrew Morton wrote:

> The general ruleset for selftests is: do as much as you can if you're not
> root and don't take too long and don't break the build on any
> architecture and don't cause the top-level "make run_tests" to fail if
> your feature is unconfigured.

This change adds a little documentation to the tests under
tools/testing/selftests/, based on akpm's explanation.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

---
 Documentation/selftests.txt |   43 ++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/Documentation/selftests.txt b/Documentation/selftests.txt
new file mode 100644
index 0000000..a00e477
--- /dev/null
+++ b/Documentation/selftests.txt
@@ -0,0 +1,43 @@
+Linux Kernel Selftests
+
+The kernel contains a set of "self tests" under the tools/testing/selftests/
+directory. These are intended to be small unit tests to exercise individual
+code paths in the kernel.
+
+Running the selftests
+=====================
+
+To build the tests:
+
+  $ make -C tools/testing/selftests
+
+
+To run the tests:
+
+  $ make -C tools/testing/selftests run_tests
+
+- note that some tests will require root privileges.
+
+
+To run only tests targetted for a single subsystem:
+
+  $  make -C tools/testing/selftests TARGETS=cpu-hotplug run_tests
+
+See the top-level tools/testing/selftests/Makefile for the list of all possible
+targets.
+
+
+Contributing new tests
+======================
+
+In general, the rules for for selftests are
+
+ * Do as much as you can if you're not root;
+
+ * Don't take too long;
+
+ * Don't break the build on any architecture, and
+
+ * Don't cause the top-level "make run_tests" to fail if your feature is
+   unconfigured.
+

Re: [PATCH] Documentation: Add a simple doc for selftests

[Andrew Morton]

On Fri, 08 Feb 2013 18:02:44 +0800
Jeremy Kerr <jk@ozlabs.org> wrote:

> On 08/02/13 07:13, Andrew Morton wrote:
> 
> > The general ruleset for selftests is: do as much as you can if you're not
> > root and don't take too long and don't break the build on any
> > architecture and don't cause the top-level "make run_tests" to fail if
> > your feature is unconfigured.
> 
> This change adds a little documentation to the tests under
> tools/testing/selftests/, based on akpm's explanation.

Looks nice, thanks ;)

I'm thinking we move it from Documentation/selftests.txt to
tools/testing/selftests/README.txt, where people are more likely to
notice it?

Re: [PATCH 1/3 v3] selftests: Add tests for efivarfs

[Jeremy Kerr]

Hi Andrew,

Thanks for taking a look at these.

>> @@ -1,4 +1,4 @@
>> -TARGETS = breakpoints kcmp mqueue vm cpu-hotplug memory-hotplug
>> +TARGETS = breakpoints kcmp mqueue vm cpu-hotplug memory-hotplug efivarfs
>
> bah.  This sort of Makefile construct is a wonderful source of patch
> rejects and fixups.  I'll covert this to
>
> --- a/tools/testing/selftests/Makefile~a
> +++ a/tools/testing/selftests/Makefile
> @@ -1,4 +1,11 @@
> -TARGETS = breakpoints epoll kcmp mqueue vm cpu-hotplug memory-hotplug efivarfs
> +TARGETS = breakpoints
> +TARGETS += epoll
> +TARGETS += kcmp
> +TARGETS += mqueue
> +TARGETS += vm
> +TARGETS += cpu-hotplug
> +TARGETS += memory-hotplug
> +TARGETS += efivarfs

Much better, thanks. I'd already had a collision with the epoll tests...

> I'll do this for now:
>
> --- a/tools/testing/selftests/efivarfs/Makefile~selftests-add-tests-for-efivarfs-fix
> +++ a/tools/testing/selftests/efivarfs/Makefile
> @@ -6,7 +6,7 @@ test_objs = open-unlink
>   all: $(test_objs)
>
>   run_tests: all
> -	@./efivarfs.sh || echo "efivarfs selftests: [FAIL]"
> +	@/bin/sh ./efivarfs.sh || echo "efivarfs selftests: [FAIL]"
>
>   clean:
>   	rm -f $(test_objs)
>
> but I'm not sure I did it right :(

efivarfs.sh requires bash currently, so we'll need to call this explicitly:

+	@/bin/bash ./efivarfs.sh || echo "efivarfs selftests: [FAIL]"

Is this okay?

> The general ruleset for selftests is: do as much as you can if you're not
> root and don't take too long and don't break the build on any
> architecture and don't cause the top-level "make run_tests" to fail if
> your feature is unconfigured.

Ah, good stuff to know. I'll send a patch adding this info to 
Documentation/ too.

> Does this code pass all that?

It should, yes:

  * all test requires root at present, as all efivarfs files are only
    writable by root

  * the built binaries doesn't use anything more than basic C, so should
    build fine wherever we have gcc.

  * efivarfs.sh will skip all tests if efivarfs is not mounted

However, the tests expose a bug at the moment, so run_tests will fail. 
Matt will have that fixed soon though :)

Cheers,


Jeremy

Re: [PATCH 1/3 v3] selftests: Add tests for efivarfs

[Matt Fleming]

On Fri, 2013-02-08 at 18:05 +0800, Jeremy Kerr wrote:
> However, the tests expose a bug at the moment, so run_tests will fail. 
> Matt will have that fixed soon though :)

In which case, would it make more sense for me to take these tests
through the efi tree? I'm fine either way, I'm just looking for the
least surprising option.

Re: [PATCH 1/3 v3] selftests: Add tests for efivarfs

[Andrew Morton]

On Fri, 08 Feb 2013 10:08:49 +0000
Matt Fleming <matt.fleming@intel.com> wrote:

> On Fri, 2013-02-08 at 18:05 +0800, Jeremy Kerr wrote:
> > However, the tests expose a bug at the moment, so run_tests will fail. 
> > Matt will have that fixed soon though :)
> 
> In which case, would it make more sense for me to take these tests
> through the efi tree? I'm fine either way, I'm just looking for the
> least surprising option.

I don't think it matters much at this stage.  selftests is new and is
used by approximately zero people so if the tests fail, few will
notice.  Let's just get it in there and get it settled down.

Re: [PATCH 1/3 v3] selftests: Add tests for efivarfs

[Matt Fleming]

On Tue, 2013-02-12 at 15:50 -0800, Andrew Morton wrote:
> On Fri, 08 Feb 2013 10:08:49 +0000
> Matt Fleming <matt.fleming@intel.com> wrote:
> 
> > On Fri, 2013-02-08 at 18:05 +0800, Jeremy Kerr wrote:
> > > However, the tests expose a bug at the moment, so run_tests will fail. 
> > > Matt will have that fixed soon though :)
> > 
> > In which case, would it make more sense for me to take these tests
> > through the efi tree? I'm fine either way, I'm just looking for the
> > least surprising option.
> 
> I don't think it matters much at this stage.  selftests is new and is
> used by approximately zero people so if the tests fail, few will
> notice.  Let's just get it in there and get it settled down.

Fine by me. Thanks Andrew.

Re: [PATCH 1/3 v3] selftests: Add tests for efivarfs

[Andrew Morton]

On Fri, 08 Feb 2013 18:05:52 +0800
Jeremy Kerr <jk@ozlabs.org> wrote:

> > I'll do this for now:
> >
> > --- a/tools/testing/selftests/efivarfs/Makefile~selftests-add-tests-for-efivarfs-fix
> > +++ a/tools/testing/selftests/efivarfs/Makefile
> > @@ -6,7 +6,7 @@ test_objs = open-unlink
> >   all: $(test_objs)
> >
> >   run_tests: all
> > -	@./efivarfs.sh || echo "efivarfs selftests: [FAIL]"
> > +	@/bin/sh ./efivarfs.sh || echo "efivarfs selftests: [FAIL]"
> >
> >   clean:
> >   	rm -f $(test_objs)
> >
> > but I'm not sure I did it right :(
> 
> efivarfs.sh requires bash currently, so we'll need to call this explicitly:
> 
> +	@/bin/bash ./efivarfs.sh || echo "efivarfs selftests: [FAIL]"
> 
> Is this okay?

Judging from ./Makefile:

# SHELL used by kbuild
CONFIG_SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
	  else if [ -x /bin/bash ]; then echo /bin/bash; \
	  else echo sh; fi ; fi)

bash is "optional" (this seems dumb, because all of us have
bash and we won't test /bin/sh).

But I expect that anyone who has an interest in running the selftests
is capable of making /bin/bash appear, so I'll make that change.