diff mbox series

[v2] selftests/user_events: Fix failures when user_events is not installed

Message ID 20230908201916.562-1-beaub@linux.microsoft.com (mailing list archive)
State Accepted
Commit a06023a8f78d3e9e73ca4363ccf3871a06e16ecc
Headers show
Series [v2] selftests/user_events: Fix failures when user_events is not installed | expand

Commit Message

Beau Belgrave Sept. 8, 2023, 8:19 p.m. UTC 
When user_events is not installed the self tests currently fail. Now
that these self tests run by default we need to ensure they don't fail
when user_events was not enabled for the kernel being tested.

Add common methods to detect if tracefs and user_events is enabled. If
either is not enabled skip the test. If tracefs is enabled, but is not
mounted, mount tracefs and fail if there were any errors. Fail if not
run as root.

Fixes: 68b4d2d58389 ("selftests/user_events: Reenable build")
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Link: https://lore.kernel.org/all/CA+G9fYuugZ0OMeS6HvpSS4nuf_A3s455ecipGBvER0LJHojKZg@mail.gmail.com/

Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
---
V2 Changes:
  Moved to stat() for existence checks vs open()/close().

 .../testing/selftests/user_events/abi_test.c  |   3 +
 .../testing/selftests/user_events/dyn_test.c  |   2 +
 .../selftests/user_events/ftrace_test.c       |   3 +
 .../testing/selftests/user_events/perf_test.c |   3 +
 .../user_events/user_events_selftests.h       | 100 ++++++++++++++++++
 5 files changed, 111 insertions(+)
 create mode 100644 tools/testing/selftests/user_events/user_events_selftests.h


base-commit: 9b1db732866bee060b9bca9493e5ebf5e8874c48

Comments

Mark Brown Sept. 8, 2023, 11:33 p.m. UTC | #1 
On Fri, Sep 08, 2023 at 08:19:16PM +0000, Beau Belgrave wrote:

> Add common methods to detect if tracefs and user_events is enabled. If
> either is not enabled skip the test. If tracefs is enabled, but is not
> mounted, mount tracefs and fail if there were any errors. Fail if not
> run as root.

This will leave tracefs mounted if it was not already mounted which is a
change to the system configuration.  While that may happen if things go
wrong during a test we should probably avoid actively doing this and
either only skip or try to umount at the end of the test if we mounted
ourselves.
Steven Rostedt Sept. 9, 2023, 1:27 a.m. UTC | #2 
On Sat, 9 Sep 2023 00:33:05 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Fri, Sep 08, 2023 at 08:19:16PM +0000, Beau Belgrave wrote:
> 
> > Add common methods to detect if tracefs and user_events is enabled. If
> > either is not enabled skip the test. If tracefs is enabled, but is not
> > mounted, mount tracefs and fail if there were any errors. Fail if not
> > run as root.  
> 
> This will leave tracefs mounted if it was not already mounted which is a
> change to the system configuration.  While that may happen if things go
> wrong during a test we should probably avoid actively doing this and
> either only skip or try to umount at the end of the test if we mounted
> ourselves.

LOL! Beau just asked me yesterday if anyone would care if the test mounted
tracefs and left it mounted. I told him "no" as ftracetest in the selftests
already do that.

I guess I was wrong and some people do care ;-)

-- Steve
Masami Hiramatsu (Google) Sept. 9, 2023, 1:57 a.m. UTC | #3 
On Sat, 9 Sep 2023 00:33:05 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Fri, Sep 08, 2023 at 08:19:16PM +0000, Beau Belgrave wrote:
> 
> > Add common methods to detect if tracefs and user_events is enabled. If
> > either is not enabled skip the test. If tracefs is enabled, but is not
> > mounted, mount tracefs and fail if there were any errors. Fail if not
> > run as root.
> 
> This will leave tracefs mounted if it was not already mounted which is a
> change to the system configuration.  While that may happen if things go
> wrong during a test we should probably avoid actively doing this and
> either only skip or try to umount at the end of the test if we mounted
> ourselves.

Oh, I didn't know that. I need to update ftracetest to unmount tracefs if
it is not mounted.

Thanks!
Beau Belgrave Sept. 12, 2023, 5:12 p.m. UTC | #4 
On Fri, Sep 08, 2023 at 09:27:12PM -0400, Steven Rostedt wrote:
> On Sat, 9 Sep 2023 00:33:05 +0100
> Mark Brown <broonie@kernel.org> wrote:
> 
> > On Fri, Sep 08, 2023 at 08:19:16PM +0000, Beau Belgrave wrote:
> > 
> > > Add common methods to detect if tracefs and user_events is enabled. If
> > > either is not enabled skip the test. If tracefs is enabled, but is not
> > > mounted, mount tracefs and fail if there were any errors. Fail if not
> > > run as root.  
> > 
> > This will leave tracefs mounted if it was not already mounted which is a
> > change to the system configuration.  While that may happen if things go
> > wrong during a test we should probably avoid actively doing this and
> > either only skip or try to umount at the end of the test if we mounted
> > ourselves.
> 
> LOL! Beau just asked me yesterday if anyone would care if the test mounted
> tracefs and left it mounted. I told him "no" as ftracetest in the selftests
> already do that.
> 
> I guess I was wrong and some people do care ;-)
> 
> -- Steve

It looks like this change got applied [1] to the fixes branch of
linux-kselftest. I can either send a V3 with this addressed or build a
patch based upon the fixes branch on top of this one to address it.

Which way do you all prefer?

Thanks,
-Beau

1. https://git.kernel.org/shuah/linux-kselftest/c/a06023a8f78d
Steven Rostedt Sept. 12, 2023, 5:46 p.m. UTC | #5 
On Tue, 12 Sep 2023 10:12:34 -0700
Beau Belgrave <beaub@linux.microsoft.com> wrote:


> > I guess I was wrong and some people do care ;-)
> > 
> > -- Steve  
> 
> It looks like this change got applied [1] to the fixes branch of
> linux-kselftest. I can either send a V3 with this addressed or build a
> patch based upon the fixes branch on top of this one to address it.
> 
> Which way do you all prefer?
> 
> Thanks,
> -Beau
> 
> 1. https://git.kernel.org/shuah/linux-kselftest/c/a06023a8f78d

I'm guessing that this would go through Shuah's tree right? So it would be
up to her to decide that.

-- Steve
Shuah Khan Sept. 15, 2023, 3:54 p.m. UTC | #6 
On 9/12/23 11:46, Steven Rostedt wrote:
> On Tue, 12 Sep 2023 10:12:34 -0700
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> 
>>> I guess I was wrong and some people do care ;-)
>>>
>>> -- Steve
>>
>> It looks like this change got applied [1] to the fixes branch of
>> linux-kselftest. I can either send a V3 with this addressed or build a
>> patch based upon the fixes branch on top of this one to address it.
>>
>> Which way do you all prefer?

Please send me patch on top of this one on linux-kselftest fixes.

thanks,
-- Shuah
Shuah Khan Sept. 15, 2023, 4:02 p.m. UTC | #7 
On 9/15/23 09:54, Shuah Khan wrote:
> On 9/12/23 11:46, Steven Rostedt wrote:
>> On Tue, 12 Sep 2023 10:12:34 -0700
>> Beau Belgrave <beaub@linux.microsoft.com> wrote:
>>
>>
>>>> I guess I was wrong and some people do care ;-)
>>>>
>>>> -- Steve
>>>
>>> It looks like this change got applied [1] to the fixes branch of
>>> linux-kselftest. I can either send a V3 with this addressed or build a
>>> patch based upon the fixes branch on top of this one to address it.
>>>
>>> Which way do you all prefer?
> 
> Please send me patch on top of this one on linux-kselftest fixes.
> 

One more thing. I am sending pull request now with this patch to address
the test failures.

Please send the unmount fix as soon as possible to include it in the next
rc.

thanks,
-- Shuah
diff mbox series

Patch

diff --git a/tools/testing/selftests/user_events/abi_test.c b/tools/testing/selftests/user_events/abi_test.c
index 5125c42efe65..22374d29ffdd 100644
--- a/tools/testing/selftests/user_events/abi_test.c
+++ b/tools/testing/selftests/user_events/abi_test.c
@@ -19,6 +19,7 @@ 
 #include <asm/unistd.h>
 
 #include "../kselftest_harness.h"
+#include "user_events_selftests.h"
 
 const char *data_file = "/sys/kernel/tracing/user_events_data";
 const char *enable_file = "/sys/kernel/tracing/events/user_events/__abi_event/enable";
@@ -93,6 +94,8 @@  FIXTURE(user) {
 };
 
 FIXTURE_SETUP(user) {
+	USER_EVENT_FIXTURE_SETUP(return);
+
 	change_event(false);
 	self->check = 0;
 }
diff --git a/tools/testing/selftests/user_events/dyn_test.c b/tools/testing/selftests/user_events/dyn_test.c
index 91a4444ad42b..32c827a52d7d 100644
--- a/tools/testing/selftests/user_events/dyn_test.c
+++ b/tools/testing/selftests/user_events/dyn_test.c
@@ -15,6 +15,7 @@ 
 #include <unistd.h>
 
 #include "../kselftest_harness.h"
+#include "user_events_selftests.h"
 
 const char *abi_file = "/sys/kernel/tracing/user_events_data";
 const char *enable_file = "/sys/kernel/tracing/events/user_events/__test_event/enable";
@@ -146,6 +147,7 @@  FIXTURE(user) {
 };
 
 FIXTURE_SETUP(user) {
+	USER_EVENT_FIXTURE_SETUP(return);
 }
 
 FIXTURE_TEARDOWN(user) {
diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c
index 5beb0aef1d81..6a260caeeddc 100644
--- a/tools/testing/selftests/user_events/ftrace_test.c
+++ b/tools/testing/selftests/user_events/ftrace_test.c
@@ -16,6 +16,7 @@ 
 #include <unistd.h>
 
 #include "../kselftest_harness.h"
+#include "user_events_selftests.h"
 
 const char *data_file = "/sys/kernel/tracing/user_events_data";
 const char *status_file = "/sys/kernel/tracing/user_events_status";
@@ -206,6 +207,8 @@  FIXTURE(user) {
 };
 
 FIXTURE_SETUP(user) {
+	USER_EVENT_FIXTURE_SETUP(return);
+
 	self->status_fd = open(status_file, O_RDONLY);
 	ASSERT_NE(-1, self->status_fd);
 
diff --git a/tools/testing/selftests/user_events/perf_test.c b/tools/testing/selftests/user_events/perf_test.c
index 8b09be566fa2..f893398cda05 100644
--- a/tools/testing/selftests/user_events/perf_test.c
+++ b/tools/testing/selftests/user_events/perf_test.c
@@ -17,6 +17,7 @@ 
 #include <asm/unistd.h>
 
 #include "../kselftest_harness.h"
+#include "user_events_selftests.h"
 
 const char *data_file = "/sys/kernel/tracing/user_events_data";
 const char *id_file = "/sys/kernel/tracing/events/user_events/__test_event/id";
@@ -113,6 +114,8 @@  FIXTURE(user) {
 };
 
 FIXTURE_SETUP(user) {
+	USER_EVENT_FIXTURE_SETUP(return);
+
 	self->data_fd = open(data_file, O_RDWR);
 	ASSERT_NE(-1, self->data_fd);
 }
diff --git a/tools/testing/selftests/user_events/user_events_selftests.h b/tools/testing/selftests/user_events/user_events_selftests.h
new file mode 100644
index 000000000000..690378942f82
--- /dev/null
+++ b/tools/testing/selftests/user_events/user_events_selftests.h
@@ -0,0 +1,100 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _USER_EVENTS_SELFTESTS_H
+#define _USER_EVENTS_SELFTESTS_H
+
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/mount.h>
+#include <unistd.h>
+#include <errno.h>
+
+#include "../kselftest.h"
+
+static inline bool tracefs_enabled(char **message, bool *fail)
+{
+	struct stat buf;
+	int ret;
+
+	*message = "";
+	*fail = false;
+
+	/* Ensure tracefs is installed */
+	ret = stat("/sys/kernel/tracing", &buf);
+
+	if (ret == -1) {
+		*message = "Tracefs is not installed";
+		return false;
+	}
+
+	/* Ensure mounted tracefs */
+	ret = stat("/sys/kernel/tracing/README", &buf);
+
+	if (ret == -1 && errno == ENOENT) {
+		if (mount(NULL, "/sys/kernel/tracing", "tracefs", 0, NULL) != 0) {
+			*message = "Cannot mount tracefs";
+			*fail = true;
+			return false;
+		}
+
+		ret = stat("/sys/kernel/tracing/README", &buf);
+	}
+
+	if (ret == -1) {
+		*message = "Cannot access tracefs";
+		*fail = true;
+		return false;
+	}
+
+	return true;
+}
+
+static inline bool user_events_enabled(char **message, bool *fail)
+{
+	struct stat buf;
+	int ret;
+
+	*message = "";
+	*fail = false;
+
+	if (getuid() != 0) {
+		*message = "Must be run as root";
+		*fail = true;
+		return false;
+	}
+
+	if (!tracefs_enabled(message, fail))
+		return false;
+
+	/* Ensure user_events is installed */
+	ret = stat("/sys/kernel/tracing/user_events_data", &buf);
+
+	if (ret == -1) {
+		switch (errno) {
+		case ENOENT:
+			*message = "user_events is not installed";
+			return false;
+
+		default:
+			*message = "Cannot access user_events_data";
+			*fail = true;
+			return false;
+		}
+	}
+
+	return true;
+}
+
+#define USER_EVENT_FIXTURE_SETUP(statement) do { \
+	char *message; \
+	bool fail; \
+	if (!user_events_enabled(&message, &fail)) { \
+		if (fail) { \
+			TH_LOG("Setup failed due to: %s", message); \
+			ASSERT_FALSE(fail); \
+		} \
+		SKIP(statement, "Skipping due to: %s", message); \
+	} \
+} while (0)
+
+#endif /* _USER_EVENTS_SELFTESTS_H */