[RFC PATCH 0/4] selftests/binderfs: Fixes to binderfs_test

[RFC PATCH 0/4] selftests/binderfs: Fixes to binderfs_test

[Abhinav Saxena]

Hi everyone,

My name is Abhinav Saxena. I am a graduate student at the University
of Calgary. This is my first patch series for the Linux kernel. I am
applying for the "Linux kernel Bug Fixing Summer Unpaid
2024". Apologies in advance if I made any trivial mistakes :)

This patch mainly includes issues reported by checkpatch.pl.  The
changes include:
- Running clang-format on `binderfs_test.c` to fix formatting issues.
- Updates the macro close_prot_errno_disarm macro.

Testing: I tested patches on my local machine (ARM64 ubuntu) with
checkpatch.pl and running the selftests.

Best,
Abhinav

Abhinav Saxena (4):
  run clang-format on bindergfs test
  update close_prot_errno_disarm macro to do{...}while(false) structure
    for safety
  Macro argument 'fd' may be better as '(fd)' to avoid precedence issues
  add missing a blank line after declarations; fix alignment formatting

 .../filesystems/binderfs/binderfs_test.c      | 204 +++++++++++-------
 1 file changed, 126 insertions(+), 78 deletions(-)

--
2.34.1

[RFC PATCH 1/4] selftests/binderfs: Run clang-format on binderfs test

[Abhinav Saxena]

This commit adds some fixes towards making
`filesystems/binderfs/binderfs_test.c` conform to the kernel coding
standards, improving readability and maintainability.

Signed-off-by: Abhinav Saxena <xandfury@gmail.com>
---
 .../filesystems/binderfs/binderfs_test.c      | 187 +++++++++++-------
 1 file changed, 116 insertions(+), 71 deletions(-)

diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index 5f362c0fd890..447ec4296e69 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -41,15 +41,16 @@ static void change_mountns(struct __test_metadata *_metadata)
 	int ret;

 	ret = unshare(CLONE_NEWNS);
-	ASSERT_EQ(ret, 0) {
+	ASSERT_EQ(ret, 0)
+	{
 		TH_LOG("%s - Failed to unshare mount namespace",
-			strerror(errno));
+		       strerror(errno));
 	}

 	ret = mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0);
-	ASSERT_EQ(ret, 0) {
-		TH_LOG("%s - Failed to mount / as private",
-			strerror(errno));
+	ASSERT_EQ(ret, 0)
+	{
+		TH_LOG("%s - Failed to mount / as private", strerror(errno));
 	}
 }

@@ -61,22 +62,25 @@ static int __do_binderfs_test(struct __test_metadata *_metadata)
 	struct binderfs_device device = { 0 };
 	struct binder_version version = { 0 };
 	char binderfs_mntpt[] = P_tmpdir "/binderfs_XXXXXX",
-		device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
-	static const char * const binder_features[] = {
+	     device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") +
+			 BINDERFS_MAX_NAME];
+	static const char *const binder_features[] = {
 		"oneway_spam_detection",
 		"extended_error",
 	};

 	change_mountns(_metadata);

-	EXPECT_NE(mkdtemp(binderfs_mntpt), NULL) {
+	EXPECT_NE(mkdtemp(binderfs_mntpt), NULL)
+	{
 		TH_LOG("%s - Failed to create binderfs mountpoint",
-			strerror(errno));
+		       strerror(errno));
 		goto out;
 	}

 	ret = mount(NULL, binderfs_mntpt, "binder", 0, 0);
-	EXPECT_EQ(ret, 0) {
+	EXPECT_EQ(ret, 0)
+	{
 		if (errno == ENODEV)
 			SKIP(goto out, "binderfs missing");
 		TH_LOG("%s - Failed to mount binderfs", strerror(errno));
@@ -87,11 +91,13 @@ static int __do_binderfs_test(struct __test_metadata *_metadata)

 	memcpy(device.name, "my-binder", strlen("my-binder"));

-	snprintf(device_path, sizeof(device_path), "%s/binder-control", binderfs_mntpt);
+	snprintf(device_path, sizeof(device_path), "%s/binder-control",
+		 binderfs_mntpt);
 	fd = open(device_path, O_RDONLY | O_CLOEXEC);
-	EXPECT_GE(fd, 0) {
+	EXPECT_GE(fd, 0)
+	{
 		TH_LOG("%s - Failed to open binder-control device",
-			strerror(errno));
+		       strerror(errno));
 		goto umount;
 	}

@@ -99,22 +105,24 @@ static int __do_binderfs_test(struct __test_metadata *_metadata)
 	saved_errno = errno;
 	close(fd);
 	errno = saved_errno;
-	EXPECT_GE(ret, 0) {
+	EXPECT_GE(ret, 0)
+	{
 		TH_LOG("%s - Failed to allocate new binder device",
-			strerror(errno));
+		       strerror(errno));
 		goto umount;
 	}

 	TH_LOG("Allocated new binder device with major %d, minor %d, and name %s",
-		device.major, device.minor, device.name);
+	       device.major, device.minor, device.name);

 	/* success: binder device allocation */

-	snprintf(device_path, sizeof(device_path), "%s/my-binder", binderfs_mntpt);
+	snprintf(device_path, sizeof(device_path), "%s/my-binder",
+		 binderfs_mntpt);
 	fd = open(device_path, O_CLOEXEC | O_RDONLY);
-	EXPECT_GE(fd, 0) {
-		TH_LOG("%s - Failed to open my-binder device",
-			strerror(errno));
+	EXPECT_GE(fd, 0)
+	{
+		TH_LOG("%s - Failed to open my-binder device", strerror(errno));
 		goto umount;
 	}

@@ -122,9 +130,10 @@ static int __do_binderfs_test(struct __test_metadata *_metadata)
 	saved_errno = errno;
 	close(fd);
 	errno = saved_errno;
-	EXPECT_GE(ret, 0) {
+	EXPECT_GE(ret, 0)
+	{
 		TH_LOG("%s - Failed to open perform BINDER_VERSION request",
-			strerror(errno));
+		       strerror(errno));
 		goto umount;
 	}

@@ -133,23 +142,26 @@ static int __do_binderfs_test(struct __test_metadata *_metadata)
 	/* success: binder transaction with binderfs binder device */

 	ret = unlink(device_path);
-	EXPECT_EQ(ret, 0) {
-		TH_LOG("%s - Failed to delete binder device",
-			strerror(errno));
+	EXPECT_EQ(ret, 0)
+	{
+		TH_LOG("%s - Failed to delete binder device", strerror(errno));
 		goto umount;
 	}

 	/* success: binder device removal */

-	snprintf(device_path, sizeof(device_path), "%s/binder-control", binderfs_mntpt);
+	snprintf(device_path, sizeof(device_path), "%s/binder-control",
+		 binderfs_mntpt);
 	ret = unlink(device_path);
-	EXPECT_NE(ret, 0) {
+	EXPECT_NE(ret, 0)
+	{
 		TH_LOG("Managed to delete binder-control device");
 		goto umount;
 	}
-	EXPECT_EQ(errno, EPERM) {
+	EXPECT_EQ(errno, EPERM)
+	{
 		TH_LOG("%s - Failed to delete binder-control device but exited with unexpected error code",
-			strerror(errno));
+		       strerror(errno));
 		goto umount;
 	}

@@ -159,9 +171,10 @@ static int __do_binderfs_test(struct __test_metadata *_metadata)
 		snprintf(device_path, sizeof(device_path), "%s/features/%s",
 			 binderfs_mntpt, binder_features[i]);
 		fd = open(device_path, O_CLOEXEC | O_RDONLY);
-		EXPECT_GE(fd, 0) {
+		EXPECT_GE(fd, 0)
+		{
 			TH_LOG("%s - Failed to open binder feature: %s",
-				strerror(errno), binder_features[i]);
+			       strerror(errno), binder_features[i]);
 			goto umount;
 		}
 		close(fd);
@@ -172,12 +185,14 @@ static int __do_binderfs_test(struct __test_metadata *_metadata)

 umount:
 	ret = umount2(binderfs_mntpt, MNT_DETACH);
-	EXPECT_EQ(ret, 0) {
+	EXPECT_EQ(ret, 0)
+	{
 		TH_LOG("%s - Failed to unmount binderfs", strerror(errno));
 	}
 rmdir:
 	ret = rmdir(binderfs_mntpt);
-	EXPECT_EQ(ret, 0) {
+	EXPECT_EQ(ret, 0)
+	{
 		TH_LOG("%s - Failed to rmdir binderfs mount", strerror(errno));
 	}
 out:
@@ -259,7 +274,8 @@ static int write_id_mapping(enum idmap_type type, pid_t pid, const char *buf,
 			return -1;

 		if (setgroups_fd >= 0) {
-			ret = write_nointr(setgroups_fd, "deny", sizeof("deny") - 1);
+			ret = write_nointr(setgroups_fd, "deny",
+					   sizeof("deny") - 1);
 			close_prot_errno_disarm(setgroups_fd);
 			if (ret != sizeof("deny") - 1)
 				return -1;
@@ -299,29 +315,34 @@ static void change_userns(struct __test_metadata *_metadata, int syncfds[2])
 	close_prot_errno_disarm(syncfds[1]);

 	ret = unshare(CLONE_NEWUSER);
-	ASSERT_EQ(ret, 0) {
+	ASSERT_EQ(ret, 0)
+	{
 		TH_LOG("%s - Failed to unshare user namespace",
-			strerror(errno));
+		       strerror(errno));
 	}

 	ret = write_nointr(syncfds[0], "1", 1);
-	ASSERT_EQ(ret, 1) {
+	ASSERT_EQ(ret, 1)
+	{
 		TH_LOG("write_nointr() failed");
 	}

 	ret = read_nointr(syncfds[0], &buf, 1);
-	ASSERT_EQ(ret, 1) {
+	ASSERT_EQ(ret, 1)
+	{
 		TH_LOG("read_nointr() failed");
 	}

 	close_prot_errno_disarm(syncfds[0]);

-	ASSERT_EQ(setid_userns_root(), 0) {
+	ASSERT_EQ(setid_userns_root(), 0)
+	{
 		TH_LOG("setid_userns_root() failed");
 	}
 }

-static void change_idmaps(struct __test_metadata *_metadata, int syncfds[2], pid_t pid)
+static void change_idmaps(struct __test_metadata *_metadata, int syncfds[2],
+			  pid_t pid)
 {
 	int ret;
 	char buf;
@@ -330,24 +351,28 @@ static void change_idmaps(struct __test_metadata *_metadata, int syncfds[2], pid
 	close_prot_errno_disarm(syncfds[0]);

 	ret = read_nointr(syncfds[1], &buf, 1);
-	ASSERT_EQ(ret, 1) {
+	ASSERT_EQ(ret, 1)
+	{
 		TH_LOG("read_nointr() failed");
 	}

 	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getuid());
 	ret = write_id_mapping(UID_MAP, pid, id_map, strlen(id_map));
-	ASSERT_EQ(ret, 0) {
+	ASSERT_EQ(ret, 0)
+	{
 		TH_LOG("write_id_mapping(UID_MAP) failed");
 	}

 	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getgid());
 	ret = write_id_mapping(GID_MAP, pid, id_map, strlen(id_map));
-	ASSERT_EQ(ret, 0) {
+	ASSERT_EQ(ret, 0)
+	{
 		TH_LOG("write_id_mapping(GID_MAP) failed");
 	}

 	ret = write_nointr(syncfds[1], "1", 1);
-	ASSERT_EQ(ret, 1) {
+	ASSERT_EQ(ret, 1)
+	{
 		TH_LOG("write_nointr() failed");
 	}

@@ -365,7 +390,7 @@ static void *binder_version_thread(void *data)
 	ret = ioctl(fd, BINDER_VERSION, &version);
 	if (ret < 0)
 		TH_LOG("%s - Failed to open perform BINDER_VERSION request\n",
-			strerror(errno));
+		       strerror(errno));

 	pthread_exit(data);
 }
@@ -385,15 +410,18 @@ TEST(binderfs_stress)
 	size_t len;
 	struct binderfs_device device = { 0 };
 	char binderfs_mntpt[] = P_tmpdir "/binderfs_XXXXXX",
-		device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
+	     device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") +
+			 BINDERFS_MAX_NAME];

 	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
-	ASSERT_EQ(ret, 0) {
+	ASSERT_EQ(ret, 0)
+	{
 		TH_LOG("%s - Failed to create socket pair", strerror(errno));
 	}

 	pid = fork();
-	ASSERT_GE(pid, 0) {
+	ASSERT_GE(pid, 0)
+	{
 		TH_LOG("%s - Failed to fork", strerror(errno));
 		close_prot_errno_disarm(syncfds[0]);
 		close_prot_errno_disarm(syncfds[1]);
@@ -406,47 +434,54 @@ TEST(binderfs_stress)
 		change_userns(_metadata, syncfds);
 		change_mountns(_metadata);

-		ASSERT_NE(mkdtemp(binderfs_mntpt), NULL) {
+		ASSERT_NE(mkdtemp(binderfs_mntpt), NULL)
+		{
 			TH_LOG("%s - Failed to create binderfs mountpoint",
-				strerror(errno));
+			       strerror(errno));
 		}

 		ret = mount(NULL, binderfs_mntpt, "binder", 0, 0);
-		ASSERT_EQ(ret, 0) {
+		ASSERT_EQ(ret, 0)
+		{
 			TH_LOG("%s - Failed to mount binderfs, check if CONFIG_ANDROID_BINDERFS is enabled in the running kernel",
-				strerror(errno));
+			       strerror(errno));
 		}

 		for (int i = 0; i < ARRAY_SIZE(fds); i++) {
-
 			snprintf(device_path, sizeof(device_path),
 				 "%s/binder-control", binderfs_mntpt);
 			fd = open(device_path, O_RDONLY | O_CLOEXEC);
-			ASSERT_GE(fd, 0) {
+			ASSERT_GE(fd, 0)
+			{
 				TH_LOG("%s - Failed to open binder-control device",
-					strerror(errno));
+				       strerror(errno));
 			}

 			memset(&device, 0, sizeof(device));
 			snprintf(device.name, sizeof(device.name), "%d", i);
 			ret = ioctl(fd, BINDER_CTL_ADD, &device);
 			close_prot_errno_disarm(fd);
-			ASSERT_EQ(ret, 0) {
+			ASSERT_EQ(ret, 0)
+			{
 				TH_LOG("%s - Failed to allocate new binder device",
-					strerror(errno));
+				       strerror(errno));
 			}

 			snprintf(device_path, sizeof(device_path), "%s/%d",
 				 binderfs_mntpt, i);
 			fds[i] = open(device_path, O_RDONLY | O_CLOEXEC);
-			ASSERT_GE(fds[i], 0) {
-				TH_LOG("%s - Failed to open binder device", strerror(errno));
+			ASSERT_GE(fds[i], 0)
+			{
+				TH_LOG("%s - Failed to open binder device",
+				       strerror(errno));
 			}
 		}

 		ret = umount2(binderfs_mntpt, MNT_DETACH);
-		ASSERT_EQ(ret, 0) {
-			TH_LOG("%s - Failed to unmount binderfs", strerror(errno));
+		ASSERT_EQ(ret, 0)
+		{
+			TH_LOG("%s - Failed to unmount binderfs",
+			       strerror(errno));
 			rmdir(binderfs_mntpt);
 		}

@@ -458,10 +493,12 @@ TEST(binderfs_stress)
 		pthread_attr_init(&attr);
 		for (k = 0; k < ARRAY_SIZE(fds); k++) {
 			for (i = 0; i < nthreads; i++) {
-				ret = pthread_create(&threads[i], &attr, binder_version_thread, INT_TO_PTR(fds[k]));
+				ret = pthread_create(&threads[i], &attr,
+						     binder_version_thread,
+						     INT_TO_PTR(fds[k]));
 				if (ret) {
 					TH_LOG("%s - Failed to create thread %d",
-						strerror(errno), i);
+					       strerror(errno), i);
 					break;
 				}
 			}
@@ -472,7 +509,8 @@ TEST(binderfs_stress)
 				ret = pthread_join(threads[j], &fdptr);
 				if (ret)
 					TH_LOG("%s - Failed to join thread %d for fd %d",
-						strerror(errno), j, PTR_TO_INT(fdptr));
+					       strerror(errno), j,
+					       PTR_TO_INT(fdptr));
 			}
 		}
 		pthread_attr_destroy(&attr);
@@ -486,7 +524,8 @@ TEST(binderfs_stress)
 	change_idmaps(_metadata, syncfds, pid);

 	ret = wait_for_pid(pid);
-	ASSERT_EQ(ret, 0) {
+	ASSERT_EQ(ret, 0)
+	{
 		TH_LOG("wait_for_pid() failed");
 	}
 }
@@ -494,10 +533,12 @@ TEST(binderfs_stress)
 TEST(binderfs_test_privileged)
 {
 	if (geteuid() != 0)
-		SKIP(return, "Tests are not run as root. Skipping privileged tests");
+		SKIP(return,
+			   "Tests are not run as root. Skipping privileged tests");

 	if (__do_binderfs_test(_metadata))
-		SKIP(return, "The Android binderfs filesystem is not available");
+		SKIP(return,
+			   "The Android binderfs filesystem is not available");
 }

 TEST(binderfs_test_unprivileged)
@@ -507,12 +548,14 @@ TEST(binderfs_test_unprivileged)
 	pid_t pid;

 	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
-	ASSERT_EQ(ret, 0) {
+	ASSERT_EQ(ret, 0)
+	{
 		TH_LOG("%s - Failed to create socket pair", strerror(errno));
 	}

 	pid = fork();
-	ASSERT_GE(pid, 0) {
+	ASSERT_GE(pid, 0)
+	{
 		close_prot_errno_disarm(syncfds[0]);
 		close_prot_errno_disarm(syncfds[1]);
 		TH_LOG("%s - Failed to fork", strerror(errno));
@@ -530,8 +573,10 @@ TEST(binderfs_test_unprivileged)
 	ret = wait_for_pid(pid);
 	if (ret) {
 		if (ret == 2)
-			SKIP(return, "The Android binderfs filesystem is not available");
-		ASSERT_EQ(ret, 0) {
+			SKIP(return,
+				   "The Android binderfs filesystem is not available");
+		ASSERT_EQ(ret, 0)
+		{
 			TH_LOG("wait_for_pid() failed");
 		}
 	}
--
2.34.1

[RFC PATCH 2/4] selftests/binderfs: Update close_prot_errno_disarm macro to do{...}while(false) structure for safety

[Abhinav Saxena]

Enclosing the close_prot_errno_disarm macro in do {...} while (false)
structure could prevent potential bugs and undefined behavior.

Example code:

    #define BINDERFUNC(x) f(x); g(x);
    if (condition)
	BINDERFUNC(x);

When BINDERFUNC(x) expands, g(x) would be executed outside of if
block. Enclosing the macro under do{...}while(false) adds a scope to
the macro, making it safer.

Signed-off-by: Abhinav Saxena <xandfury@gmail.com>
---
 .../filesystems/binderfs/binderfs_test.c         | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index 447ec4296e69..4a149e3d4ba4 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -28,13 +28,15 @@
 #define PTR_TO_INT(p) ((int)((intptr_t)(p)))
 #define INT_TO_PTR(u) ((void *)((intptr_t)(u)))

-#define close_prot_errno_disarm(fd) \
-	if (fd >= 0) {              \
-		int _e_ = errno;    \
-		close(fd);          \
-		errno = _e_;        \
-		fd = -EBADF;        \
-	}
+#define close_prot_errno_disarm(fd)      \
+	do {				 \
+		if (fd >= 0) {		 \
+			int _e_ = errno; \
+			close(fd);	 \
+			errno = _e_;	 \
+			fd = -EBADF;	 \
+		}			 \
+	} while (false)

 static void change_mountns(struct __test_metadata *_metadata)
 {
--
2.34.1

[RFC PATCH 3/4] selftests/binderfs: Macro argument 'fd' may be better as '(fd)' to avoid precedence issues

[Abhinav Saxena]

Change the macro argument 'fd' to '(fd)' to avoid potential precedence
issues. Without parentheses, the macro expansion could lead to
unexpected behavior when used with an expression having different
precedence levels.

Example Code:

    #define CALC_SQR_BAD(x) (x*x)
    #define CALC_SQR_GOOD(x) ((x)*(x))

CALC_SQR_BAD(2+3) expands to 2+(3*3)+2, giving 11 as the incorrect
answer. Enclosing with parathesis CALC_SQR_GOOD(2+3) sets the correct
order of precedence and expands to (2+3)*(2+3), giving 25 as the
correct answer.

Signed-off-by: Abhinav Saxena <xandfury@gmail.com>
---
 .../testing/selftests/filesystems/binderfs/binderfs_test.c  | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index 4a149e3d4ba4..d5dba6c1e07f 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -30,11 +30,11 @@

 #define close_prot_errno_disarm(fd)      \
 	do {				 \
-		if (fd >= 0) {		 \
+		if ((fd) >= 0) {	 \
 			int _e_ = errno; \
-			close(fd);	 \
+			close((fd));	 \
 			errno = _e_;	 \
-			fd = -EBADF;	 \
+			(fd) = -EBADF;	 \
 		}			 \
 	} while (false)

--
2.34.1

[RFC PATCH 4/4] selftests/binderfs: Add missing a blank line after declarations

[Abhinav Saxena]

Adds a blank line after declarations and fixes some more formatting
issues.

Signed-off-by: Abhinav Saxena <xandfury@gmail.com>
---
 .../testing/selftests/filesystems/binderfs/binderfs_test.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index d5dba6c1e07f..d8a22b62dd89 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -433,6 +433,7 @@ TEST(binderfs_stress)
 		int i, j, k, nthreads;
 		pthread_attr_t attr;
 		pthread_t threads[DEFAULT_THREADS];
+
 		change_userns(_metadata, syncfds);
 		change_mountns(_metadata);

@@ -536,11 +537,11 @@ TEST(binderfs_test_privileged)
 {
 	if (geteuid() != 0)
 		SKIP(return,
-			   "Tests are not run as root. Skipping privileged tests");
+		     "Tests are not run as root. Skipping privileged tests");

 	if (__do_binderfs_test(_metadata))
 		SKIP(return,
-			   "The Android binderfs filesystem is not available");
+		     "The Android binderfs filesystem is not available");
 }

 TEST(binderfs_test_unprivileged)
@@ -576,7 +577,7 @@ TEST(binderfs_test_unprivileged)
 	if (ret) {
 		if (ret == 2)
 			SKIP(return,
-				   "The Android binderfs filesystem is not available");
+			     "The Android binderfs filesystem is not available");
 		ASSERT_EQ(ret, 0)
 		{
 			TH_LOG("wait_for_pid() failed");
--
2.34.1

Re: [RFC PATCH 0/4] selftests/binderfs: Fixes to binderfs_test

[Abhinav Saxena]

Adding Shuah Khan<skhan@linuxfoundation.org> to cc.

-Abhinav.

On Wed, May 15, 2024 at 01:08:01AM +0000, Abhinav Saxena wrote:
> Hi everyone,
> 
> My name is Abhinav Saxena. I am a graduate student at the University
> of Calgary. This is my first patch series for the Linux kernel. I am
> applying for the "Linux kernel Bug Fixing Summer Unpaid
> 2024". Apologies in advance if I made any trivial mistakes :)
> 
> This patch mainly includes issues reported by checkpatch.pl.  The
> changes include:
> - Running clang-format on `binderfs_test.c` to fix formatting issues.
> - Updates the macro close_prot_errno_disarm macro.
> 
> Testing: I tested patches on my local machine (ARM64 ubuntu) with
> checkpatch.pl and running the selftests.
> 
> Best,
> Abhinav
> 
> Abhinav Saxena (4):
>   run clang-format on bindergfs test
>   update close_prot_errno_disarm macro to do{...}while(false) structure
>     for safety
>   Macro argument 'fd' may be better as '(fd)' to avoid precedence issues
>   add missing a blank line after declarations; fix alignment formatting
> 
>  .../filesystems/binderfs/binderfs_test.c      | 204 +++++++++++-------
>  1 file changed, 126 insertions(+), 78 deletions(-)
> 
> --
> 2.34.1
> 

Re: [RFC PATCH 3/4] selftests/binderfs: Macro argument 'fd' may be better as '(fd)' to avoid precedence issues

[Bill Wendling]

On Tue, May 14, 2024 at 6:08 PM Abhinav Saxena <xandfury@gmail.com> wrote:
>
> Change the macro argument 'fd' to '(fd)' to avoid potential precedence
> issues. Without parentheses, the macro expansion could lead to
> unexpected behavior when used with an expression having different
> precedence levels.
>
> Example Code:
>
>     #define CALC_SQR_BAD(x) (x*x)
>     #define CALC_SQR_GOOD(x) ((x)*(x))
>
> CALC_SQR_BAD(2+3) expands to 2+(3*3)+2, giving 11 as the incorrect
> answer. Enclosing with parathesis CALC_SQR_GOOD(2+3) sets the correct

s/parathesis/parentheses/

> order of precedence and expands to (2+3)*(2+3), giving 25 as the
> correct answer.
>
> Signed-off-by: Abhinav Saxena <xandfury@gmail.com>
> ---
>  .../testing/selftests/filesystems/binderfs/binderfs_test.c  | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> index 4a149e3d4ba4..d5dba6c1e07f 100644
> --- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> @@ -30,11 +30,11 @@
>
>  #define close_prot_errno_disarm(fd)      \
>         do {                             \
> -               if (fd >= 0) {           \
> +               if ((fd) >= 0) {         \
>                         int _e_ = errno; \
> -                       close(fd);       \
> +                       close((fd));     \
>                         errno = _e_;     \
> -                       fd = -EBADF;     \
> +                       (fd) = -EBADF;   \

While I agree that it's generally good to add parentheses in macros,
this line ensures that 'fd' must be an lvalue and not an arithmetic
expression.

-bw

>                 }                        \
>         } while (false)
>
> --
> 2.34.1
>