throne_tracker: skip iterate if failed to open dir

[5ec1cff]

fix #1800

[aviraxp]

@gooroodev can you review this?

[gooroodev]

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!