Skip to content

Conversation

@vicLin8712
Copy link
Collaborator

@vicLin8712 vicLin8712 commented Nov 26, 2025

The previous implementation compared kcb->task_current with the task’s list node in the global task list. A list node is only a linkage element, not a stable identifier for a task, so this comparison is not appropriate.

This patch updates the check to compare the task's data structure (TCB), which properly represents the task identity.


Summary by cubic

Compare the task’s TCB instead of its list node in mo_task_suspend() when checking if the target is the running task. This clarifies intent and avoids using list linkage as identity; behavior is unchanged.

Written for commit bf76c93. Summary will update automatically on new commits.

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

@jserv jserv requested a review from HeatCrab November 26, 2025 04:46
@jserv
Copy link
Contributor

jserv commented Nov 26, 2025

I defer to @HeatCrab for confirmation.

@HeatCrab
Copy link
Collaborator

LGTM!

@visitorckw
Copy link
Collaborator

I'm curious, under what circumstances would (kcb->task_current->data == task) != (kcb->task_current == node) ?

@vicLin8712
Copy link
Collaborator Author

I'm curious, under what circumstances would (kcb->task_current->data == task) != (kcb->task_current == node) ?

Generally, there is no possibility that your inequivalent happens in the current codebase.

The intention of this patch is not to fix a real bug, but to correct the semantics so that the check aligns with the meaning of the is_current. The is_current parameter is conceptually about whether the task being operated on is the currently running task, not whether the list node happens to match task_current.

So the change is aiming to reflect the intended abstraction (task-level comparison), even though both expressions are equivalent in the current code base.

If I misunderstood the intended meaning of is_current (e.g., if it was supposed to be node-based), please let me know.

@visitorckw
Copy link
Collaborator

Thanks for the clarification.

Although I noticed the title starts with "Refine" instead of "Fix", the wording in the description left me a bit confused as to whether:

a) In rare cases, (kcb->task_current->data == task) != (kcb->task_current == node) could actually happen.
b) The equality always holds, and this is merely to improve code readability.

It turns out to be the latter. However, I would prefer the commit message to be more explicit that the mismatch never actually occurs, and that this change is purely for readability.

The previous implementation compared `kcb->task_current` against the
task's list node. This is always equivalent to comparing the task's TCB
in the current implementation, so no functional behavior is affected and
the mismatch can never occur.

This change simply makes the intended semantics explicit by comparing
against the TCB directly, improving readability and clarifying the
abstraction.
@vicLin8712 vicLin8712 force-pushed the fix-current-task-check branch from 5d27cea to bf76c93 Compare November 26, 2025 09:02
@vicLin8712
Copy link
Collaborator Author

Thanks for the clarification.

Although I noticed the title starts with "Refine" instead of "Fix", the wording in the description left me a bit confused as to whether:

a) In rare cases, (kcb->task_current->data == task) != (kcb->task_current == node) could actually happen. b) The equality always holds, and this is merely to improve code readability.

It turns out to be the latter. However, I would prefer the commit message to be more explicit that the mismatch never actually occurs, and that this change is purely for readability.

Thanks for the feedback.

I've updated the commit message to clarify that the two conditions are always equivalent and that this change is purely for readability.

@jserv jserv merged commit 295a0d9 into sysprog21:main Nov 26, 2025
6 checks passed
@jserv
Copy link
Contributor

jserv commented Nov 26, 2025

Thank @vicLin8712 for contributing!

@vicLin8712 vicLin8712 deleted the fix-current-task-check branch November 26, 2025 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants