throne_tracker: skip iterate if failed to open dir by 5ec1cff · Pull Request #1832 · tiann/KernelSU

5ec1cff · 2024-06-18T15:56:20Z

aviraxp · 2024-07-02T15:32:01Z

@gooroodev can you review this?

gooroodev · 2024-07-02T15:32:58Z

Thanks, @aviraxp! Let's get started!

Issue Counts Table

🐞Mistake	🤪Typo	🚨Security	🚀Performance	💪Best Practices	📖Readability	❓Others
0	0	0	0	1	1	0

Summary of Proposed Changes

🛠️ Changed the error handling mechanism to skip iteration instead of returning immediately when failing to open a directory.

Identified Issues

ID	Type	Details	Severity	Confidence
1	💪Best Practices	Use of `goto` statement for error handling is generally discouraged.	🟠Medium	🟠Medium
2	📖Readability	The label `skip_iterate` could be more descriptive.	🟡Low	🟡Low

Issue 1: Use of `goto` statement for error handling is generally discouraged

Explanation

Using goto for error handling can make the code harder to read and maintain. It is generally better to use structured control flow constructs like loops and conditionals.

Suggested Code Change

diff --git a/kernel/throne_tracker.c b/kernel/throne_tracker.c
index eb04c8b0f344..d7c1dae14a17 100644
--- a/kernel/throne_tracker.c
+++ b/kernel/throne_tracker.c
@@ -236,13 +236,14 @@ void search_manager(const char *path, int depth, struct list_head *uid_data)
 				file = ksu_filp_open_compat(pos->dirpath, O_RDONLY | O_NOFOLLOW, 0);
 				if (IS_ERR(file)) {
 					pr_err("Failed to open directory: %s, err: %ld\n", pos->dirpath, PTR_ERR(file));
-					goto skip_iterate;
+					list_del(&pos->list);
+					if (pos != &data)
+						kfree(pos);
+					continue;
 				}
 
 				iterate_dir(file, &ctx.ctx);
 				filp_close(file, NULL);
 			}
-
-			list_del(&pos->list);
-			if (pos != &data)
-				kfree(pos);
+			list_del(&pos->list);
+			if (pos != &data)
+				kfree(pos);
 		}

Explanation of the Fix

This change removes the goto statement and replaces it with structured control flow. If the directory cannot be opened, the code now directly deletes the list entry and continues to the next iteration.

Issue 2: The label `skip_iterate` could be more descriptive

Explanation

The label skip_iterate is not very descriptive and could be improved for better readability.

Suggested Code Change

If the goto statement is retained, a more descriptive label could be used:

diff --git a/kernel/throne_tracker.c b/kernel/throne_tracker.c
index eb04c8b0f344..d7c1dae14a17 100644
--- a/kernel/throne_tracker.c
+++ b/kernel/throne_tracker.c
@@ -236,13 +236,13 @@ void search_manager(const char *path, int depth, struct list_head *uid_data)
 				file = ksu_filp_open_compat(pos->dirpath, O_RDONLY | O_NOFOLLOW, 0);
 				if (IS_ERR(file)) {
 					pr_err("Failed to open directory: %s, err: %ld\n", pos->dirpath, PTR_ERR(file));
-					goto skip_iterate;
+					goto cleanup_and_continue;
 				}
 
 				iterate_dir(file, &ctx.ctx);
 				filp_close(file, NULL);
 			}
-
+cleanup_and_continue:
 			list_del(&pos->list);
 			if (pos != &data)
 				kfree(pos);

Explanation of the Fix

The label skip_iterate is changed to cleanup_and_continue to make it clearer what the goto statement is intended to do.

General Review of Code Quality and Style

The code quality is generally good, but the use of goto for error handling is not ideal. Structured control flow should be preferred. The readability could be slightly improved by using more descriptive labels if goto is retained.

--
I only arrive when I am mentioned and asked to review the pull request.
React or reply to let me know your opinion!

fix tiann#1800

* 'main' of https://github.com/tiann/KernelSU: build(deps): bump the npm group in /website with 3 updates (tiann#1903) build(deps): bump syn from 2.0.71 to 2.0.72 in /userspace/ksud in the crates group (tiann#1902) Upgrade rustix (tiann#1900) build(deps): bump the npm group in /website with 2 updates (tiann#1899) build(deps): bump the crates group in /userspace/ksud with 3 updates (tiann#1898) build(deps): bump the npm group in /website with 13 updates (tiann#1893) build(deps): bump the crates group in /userspace/ksud with 40 updates (tiann#1894) [skip ci] Group dependabot dependencies (tiann#1892) Upgrade zip (tiann#1891) build(deps): bump io.coil-kt:coil-compose from 2.6.0 to 2.7.0 in /manager (tiann#1888) build(deps): bump org.lsposed.libcxx:libcxx from 27.0.11718014-beta1 to 27.0.12077973 in /manager (tiann#1885) Upgrade deps (tiann#1886) build(deps-dev): bump vitepress from 1.3.0 to 1.3.1 in /website (tiann#1879) build(deps): bump com.google.devtools.ksp from 2.0.0-1.0.22 to 2.0.0-1.0.23 in /manager (tiann#1872) build(deps): bump agp from 8.5.0 to 8.5.1 in /manager (tiann#1873) Allow skipping commented policy (tiann#1870) build(deps): bump rust-embed from 8.4.0 to 8.5.0 in /userspace/ksud (tiann#1869) build(deps): bump clap from 4.5.8 to 4.5.9 in /userspace/ksud (tiann#1868) Setup Android SDK (tiann#1867) Upgrade gradle (tiann#1866) build(deps-dev): bump vitepress from 1.2.3 to 1.3.0 in /website (tiann#1865) Revert "ksud: [Fix] grant root to the shell in debug mode" (tiann#1860) ksud: upgrade zip (tiann#1859) ksud: [Fix] grant root to the shell in debug mode (tiann#1853) build(deps): bump lifecycle from 2.8.1 to 2.8.3 in /manager (tiann#1851) build(deps): bump clap from 4.5.7 to 4.5.8 in /userspace/ksud (tiann#1850) website: fix typo (tiann#1834) build(deps): bump android_logger from 0.13.3 to 0.14.1 in /userspace/ksud (tiann#1830) build(deps): bump log from 0.4.21 to 0.4.22 in /userspace/ksud (tiann#1843) Update resetprop (tiann#1842) throne_tracker: skip iterate if failed to open dir (tiann#1832) Translations update from Hosted Weblate (tiann#1734) build(deps): bump com.google.devtools.ksp from 2.0.0-1.0.21 to 2.0.0-1.0.22 in /manager (tiann#1811) manager: improve grammar in english (tiann#1814) Redirect the Feature Requests issue template tab (tiann#1788) website: fix typo (tiann#1807) build(deps): bump zip from 2.1.2 to 2.1.3 in /userspace/ksud (tiann#1810) build(deps): bump regex from 1.10.4 to 1.10.5 in /userspace/ksud (tiann#1815) build(deps): bump clap from 4.5.4 to 4.5.7 in /userspace/ksud (tiann#1817) build(deps): bump androidx.compose:compose-bom from 2024.05.00 to 2024.06.00 in /manager (tiann#1820) build(deps): bump agp from 8.4.1 to 8.5.0 in /manager (tiann#1824) build(deps): bump lifecycle from 2.8.0 to 2.8.1 in /manager (tiann#1782) website: update translations (tiann#1796) build(deps): bump zip-extensions from 0.6.2 to 0.7.0 in /userspace/ksud (tiann#1798) build(deps): bump zip from 2.1.0 to 2.1.2 in /userspace/ksud (tiann#1799) build(deps-dev): bump vitepress from 1.2.2 to 1.2.3 in /website (tiann#1803) manager: fix update no response when changelog is empty (tiann#1786) Convert devpts domain to ksu_file (tiann#1801) non-GKI:Remove maintainer Coconutat's repositories (tiann#1794) 从非官方支持设备中移除K-Nel-M1721 (tiann#1792) Signed-off-by: Edwiin Kusuma Jaya <[email protected]>

fix tiann#1800

This corresponds to v0.9.5 / 11872 upstream with the cherry-picks from upstream: Convert devpts domain to ksu_file (tiann#1801) throne_tracker: skip iterate if failed to open dir (tiann#1832) kernel: Relax dentry_path_raw compare in core_hook (tiann#2041) kernel/core_hook.c: use upstream ksu_umount_mnt kernel: require path_umount backporting Addendums for the sake of legacy maintenance: treewide: tree prep for kernel-driver only kernel: expose allowlist workaround as Kconfig option kernel: selinux: add security_bounded_transition hook for < 4.14 kernel: selinux: fix redefined KERNEL_SU_DOMAIN kernel/ksu.c: disable KPROBES=n warning KernelSU: 11872-klts Warning: Kernel built with this branch will not work with the provided manager from this repo. Use KernelSU v1.0.1 Manager. Signed-off-by: backslashxx <[email protected]>

This corresponds to v0.9.5 / 11872 upstream with the cherry-picks from upstream: Convert devpts domain to ksu_file (tiann#1801) throne_tracker: skip iterate if failed to open dir (tiann#1832) kernel: Relax dentry_path_raw compare in core_hook (tiann#2041) kernel/core_hook.c: use upstream ksu_umount_mnt kernel: require path_umount backporting Addendums for the sake of legacy maintenance: treewide: tree prep for kernel-driver only kernel: expose allowlist workaround as Kconfig option kernel: selinux: add security_bounded_transition hook for < 4.14 kernel: selinux: fix redefined KERNEL_SU_DOMAIN kernel/ksu.c: disable KPROBES=n warning KernelSU: 11872-klts Warning: Kernel built with this branch will not work with the provided manager from this repo. Use official KernelSU v1.0.1 Manager. Signed-off-by: backslashxx <[email protected]>

This corresponds to v0.9.5 / 11872 upstream with cherry-picks from upstream: Convert devpts domain to ksu_file (tiann#1801) throne_tracker: skip iterate if failed to open dir (tiann#1832) kernel: Relax dentry_path_raw compare in core_hook (tiann#2041) kernel/core_hook.c: use upstream ksu_umount_mnt kernel: require path_umount backporting Addendums for the sake of legacy maintenance: treewide: tree prep for kernel-driver only kernel: expose allowlist workaround as Kconfig option kernel: selinux: add security_bounded_transition hook for < 4.14 kernel: selinux: fix redefined KERNEL_SU_DOMAIN kernel/ksu.c: disable KPROBES=n warning KernelSU: 11872-klts Warning: Kernel built with this branch will not work with the provided manager from this repo. Use official KernelSU v1.0.1 Manager. Signed-off-by: backslashxx <[email protected]>

fix tiann#1800

throne_tracker: skip iterate if failed to open dir

e53ba64

yujincheng08 approved these changes Jun 18, 2024

View reviewed changes

tiann merged commit f52beb9 into tiann:main Jun 19, 2024

5ec1cff deleted the patch-3 branch June 19, 2024 01:24

whyakari pushed a commit to whyakari/KernelSU that referenced this pull request Jul 7, 2024

throne_tracker: skip iterate if failed to open dir (tiann#1832)

358768e

fix tiann#1800

itsHanibee pushed a commit to Dominium-Apum/kernelsu that referenced this pull request Jul 16, 2024

throne_tracker: skip iterate if failed to open dir (tiann#1832)

dc47c21

fix tiann#1800

xxmustafacooTR pushed a commit to xxPlayground/KernelSU that referenced this pull request Jul 24, 2024

throne_tracker: skip iterate if failed to open dir (tiann#1832)

ac30ccb

fix tiann#1800

Jbub5 pushed a commit to Jbub5/KernelSU that referenced this pull request Jul 25, 2024

throne_tracker: skip iterate if failed to open dir (tiann#1832)

86678a1

fix tiann#1800

bigfoxtail mentioned this pull request Jul 29, 2024

Kernelsu not working crdroidandroid/issue_tracker#474

Closed

5 tasks

fukiame pushed a commit to TelegramAt25/KernelSU-shukusai that referenced this pull request Aug 11, 2024

throne_tracker: skip iterate if failed to open dir (tiann#1832)

6114966

fix tiann#1800

LeCmnGend pushed a commit to LeCmnGend/KernelSU that referenced this pull request Sep 25, 2024

throne_tracker: skip iterate if failed to open dir (tiann#1832)

70c3ec7

fix tiann#1800

revonateB0T pushed a commit to revonateB0T/KernelSU-LTS that referenced this pull request Oct 10, 2024

throne_tracker: skip iterate if failed to open dir (tiann#1832)

833a878

fix tiann#1800

backslashxx pushed a commit to backslashxx/KernelSU that referenced this pull request Oct 20, 2024

throne_tracker: skip iterate if failed to open dir (tiann#1832)

67a111d

fix tiann#1800

backslashxx pushed a commit to backslashxx/KernelSU that referenced this pull request Nov 14, 2024

throne_tracker: skip iterate if failed to open dir (tiann#1832)

8acc57a

fix tiann#1800

backslashxx pushed a commit to backslashxx/KernelSU that referenced this pull request Nov 27, 2024

throne_tracker: skip iterate if failed to open dir (tiann#1832)

8ae4c11

fix tiann#1800

backslashxx pushed a commit to backslashxx/KernelSU that referenced this pull request Dec 7, 2024

throne_tracker: skip iterate if failed to open dir (tiann#1832)

4c3aa38

fix tiann#1800

itswill00 pushed a commit to itswill00/KernelSU-Next-4.14 that referenced this pull request Jan 19, 2025

throne_tracker: skip iterate if failed to open dir (tiann#1832)

d3e0353

fix tiann#1800

backslashxx pushed a commit to backslashxx/KernelSU that referenced this pull request Feb 3, 2025

throne_tracker: skip iterate if failed to open dir (tiann#1832)

6d50aa7

fix tiann#1800

Coffee-0xFF pushed a commit to Coffee-0xFF/KernelSU that referenced this pull request Mar 19, 2025

throne_tracker: skip iterate if failed to open dir (tiann#1832)

27bf9b7

fix tiann#1800

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

throne_tracker: skip iterate if failed to open dir#1832

throne_tracker: skip iterate if failed to open dir#1832
tiann merged 1 commit intotiann:mainfrom
5ec1cff:patch-3

5ec1cff commented Jun 18, 2024 •

edited

Loading

Uh oh!

aviraxp commented Jul 2, 2024

Uh oh!

gooroodev commented Jul 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Comments

Conversation

5ec1cff commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aviraxp commented Jul 2, 2024

Uh oh!

gooroodev commented Jul 2, 2024

Issue Counts Table

Summary of Proposed Changes

Identified Issues

Issue 1: Use of goto statement for error handling is generally discouraged

Explanation

Suggested Code Change

Explanation of the Fix

Issue 2: The label skip_iterate could be more descriptive

Explanation

Suggested Code Change

Explanation of the Fix

General Review of Code Quality and Style

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

5ec1cff commented Jun 18, 2024 •

edited

Loading

Issue 1: Use of `goto` statement for error handling is generally discouraged

Issue 2: The label `skip_iterate` could be more descriptive