Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore missing break in process_link_channels() #448

Conversation

mkmer
Copy link
Collaborator

@mkmer mkmer commented Jan 9, 2025

Addressing issue #447
Restore in inadvertently removed break

@Allan-N Allan-N added the bug Something isn't working label Jan 9, 2025
@Allan-N Allan-N linked an issue Jan 9, 2025 that may be closed by this pull request
@Allan-N
Copy link
Collaborator

Allan-N commented Jan 9, 2025

remote_hangup_helper() is an odd one. The function looks to expect that the passed in myrpt->lock is NOT locked but exits with the lock held.

@InterLinked1
Copy link
Member

remote_hangup_helper() is an odd one. The function looks to expect that the passed in myrpt->lock is NOT locked but exits with the lock held.

Yeah, agreed, probably not my best thinking on that one, I took all the identical code and moved it in there without paying attention to that detail.

Copy link
Member

@InterLinked1 InterLinked1 left a comment

Choose a reason for hiding this comment

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

Please replace your commit message with the following, amend your commit, and force push it:

app_rpt: Readd missing break statement.

Restore a break statement that was accidentally removed during the refactor in
commit 3834be01031a4098a14b2c36f99f99a26223f396.

Fixes: #447

@Allan-N
Copy link
Collaborator

Allan-N commented Jan 9, 2025

Please replace your commit message

Translated ...

  • checkout your branch
  • git commit --amend
  • update the commit comment
  • git push --force

Restore a break statement that was accidentally removed during the refactor in
commit 3834be0.

Fixes: AllStarLink#447
@mkmer mkmer force-pushed the Restore-missing-break-in-process-link-channels branch from 460fa4a to e4634d7 Compare January 10, 2025 00:42
@mkmer
Copy link
Collaborator Author

mkmer commented Jan 10, 2025

Sorry - other projects I have contributed to put these in the PR rather than commit. Now I know for the next one ;)

@mkmer
Copy link
Collaborator Author

mkmer commented Jan 10, 2025

remote_hangup_helper() is an odd one. The function looks to expect that the passed in myrpt->lock is NOT locked but exits with the lock held.

Yeah, agreed, probably not my best thinking on that one, I took all the identical code and moved it in there without paying attention to that detail.

I did do a bit more digging - with the break it now appears to match the original code, but that doesn't mean it was right in the first place :)
Just wanted to share my observations - some times that new guy has a dumb question that everyone missed.

@InterLinked1
Copy link
Member

remote_hangup_helper() is an odd one. The function looks to expect that the passed in myrpt->lock is NOT locked but exits with the lock held.

Yeah, agreed, probably not my best thinking on that one, I took all the identical code and moved it in there without paying attention to that detail.

I did do a bit more digging - with the break it now appears to match the original code, but that doesn't mean it was right in the first place :) Just wanted to share my observations - some times that new guy has a dumb question that everyone missed.

Glad you did some digging - you uncovered something important here we had all missed until now! It's much appreciated.

The PR description is nice, but really it's the commit that matters since that's what's merged and what people will see when they look at the repo's history.

@Allan-N Allan-N requested a review from InterLinked1 January 10, 2025 01:19
@InterLinked1 InterLinked1 merged commit 96ad6df into AllStarLink:master Jan 11, 2025
1 check failed
@mkmer mkmer deleted the Restore-missing-break-in-process-link-channels branch January 11, 2025 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double frame free in rpt_app.c?
4 participants