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

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

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