[PATCH 0/3] selftests: Add efivarfs tests

[PATCH 0/3] 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

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

[PATCH 1/3] 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>.

Signed-off-by: Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
CC: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: Lingzhu Xiang <lxiang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.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 4348014..921dcdc 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -1,4 +1,4 @@
-TARGETS = breakpoints kcmp mqueue vm cpu-hotplug memory-hotplug epoll
+TARGETS = breakpoints kcmp mqueue vm cpu-hotplug memory-hotplug epoll 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;
+}

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

[Jeremy Kerr]

Signed-off-by: Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.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..25b6b77
--- /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 && errno == EIO)) {
+		fprintf(stderr, "Reading a new var should return -EIO\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

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

[Lingzhu Xiang]

On 01/27/2013 06:47 AM, Jeremy Kerr wrote:
> +	if (!(rc < 0 && errno == EIO)) {
> +		fprintf(stderr, "Reading a new var should return -EIO\n");

What does the -EIO imply for reading a known empty file?

The file is empty. There is nothing to be read. No IO should actually 
happens.
The variable doesn't exist in nvram. What is the error? This won't make 
sense
for userspace.

Empty variable never exists in nvram per spec. Userspace doesn't need an 
extra
EIO to figure out this known fact. In the meantime, user would wonder if
something else has gone wrong? Returning zero for reading an empty file can
reserve EIO for something more informational.

Maybe at this time we just leave it as it is for other reasons. But at least
I don't think it's a good idea to mandate this behavior.

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

[Matt Fleming]

On Mon, 2013-01-28 at 19:25 +0800, Lingzhu Xiang wrote:
> On 01/27/2013 06:47 AM, Jeremy Kerr wrote:
> > +	if (!(rc < 0 && errno == EIO)) {
> > +		fprintf(stderr, "Reading a new var should return -EIO\n");
> 
> What does the -EIO imply for reading a known empty file?
> 
> The file is empty. There is nothing to be read. No IO should actually 
> happens.
> The variable doesn't exist in nvram. What is the error? This won't make 
> sense
> for userspace.

I was the one who originally suggested using EIO for reading a variable
that hasn't hit the backing store, mainly because it simplified the
EFI_NOT_FOUND logic, and I felt that reading from a non-existent
variable should be an error condition. But I can see the case for
returning zero instead.

> Empty variable never exists in nvram per spec. Userspace doesn't need an 
> extra
> EIO to figure out this known fact. In the meantime, user would wonder if
> something else has gone wrong? Returning zero for reading an empty file can
> reserve EIO for something more informational.

Perhaps it's not unreasonable to expect users to understand that
zero-length files don't have a corresponding EFI variable, and that the
file won't persist across a reboot.

Jeremy, thoughts?

> Maybe at this time we just leave it as it is for other reasons. But at least
> I don't think it's a good idea to mandate this behavior.

I do think it's important that we hash out these semantics as soon as
possible, before userspace starts relying on them.

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Jeremy Kerr]

Hi all,

>> Empty variable never exists in nvram per spec. Userspace doesn't need an
>> extra
>> EIO to figure out this known fact. In the meantime, user would wonder if
>> something else has gone wrong? Returning zero for reading an empty file can
>> reserve EIO for something more informational.
>
> Perhaps it's not unreasonable to expect users to understand that
> zero-length files don't have a corresponding EFI variable, and that the
> file won't persist across a reboot.
>
> Jeremy, thoughts?

I think that EOF would be better; it's not really an "error" condition, 
just that no data has been given yet.

I'd be happy to change this check to suit.

Cheers,


Jeremy

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

[Matt Fleming]

On Tue, 2013-01-29 at 07:40 +0900, Jeremy Kerr wrote:
> Hi all,
> 
> >> Empty variable never exists in nvram per spec. Userspace doesn't need an
> >> extra
> >> EIO to figure out this known fact. In the meantime, user would wonder if
> >> something else has gone wrong? Returning zero for reading an empty file can
> >> reserve EIO for something more informational.
> >
> > Perhaps it's not unreasonable to expect users to understand that
> > zero-length files don't have a corresponding EFI variable, and that the
> > file won't persist across a reboot.
> >
> > Jeremy, thoughts?
> 
> I think that EOF would be better; it's not really an "error" condition, 
> just that no data has been given yet.

Works for me.

> I'd be happy to change this check to suit.

That would be great, thank you.

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Lingzhu Xiang]

On 01/28/2013 08:37 PM, Matt Fleming wrote:
> I was the one who originally suggested using EIO for reading a variable
> that hasn't hit the backing store, mainly because it simplified the
> EFI_NOT_FOUND logic, and I felt that reading from a non-existent
> variable should be an error condition. But I can see the case for
> returning zero instead.

I agree translating EFI_NOT_FOUND to EIO is generally a good choice.
But in the special case of an uncommitted empty file, kernel can avoid
reading the expectedly non-existent variable and instead return
EOF directly.

Yes, empty files are uncommitted and will go away after reboot anyway,
regardless of EIO or not. Users have to know this.

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

[Jeremy Kerr]

Signed-off-by: Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.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