ospfd: Do not turn on write thread unless we have something in it #4907
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-8745/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
qlyoung
reviewed
Aug 30, 2019
I am rarely seeing this crash: r2: ospfd crashed. Core file found - Backtrace follows: [New LWP 32748] [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/aarch64-linux-gnu/libthread_db.so.1". Core was generated by `/usr/lib/frr/ospfd'. Program terminated with signal SIGABRT, Aborted. 2019-08-29 15:59:36,149 ERROR: assert failed at "test_ospf_sr_topo1/test_memory_leak": Which translates to this code: node = listhead(ospf->oi_write_q); assert(node); oi = listgetdata(node); assert(oi); So if we get into ospf_write without anything on the oi_write_q we are stopping the program. This is happening because in ospf_ls_upd_queue_send we are calling ospf_write. Imagine that we have a interface already on the on_write_q and then ospf_write handles the packet send for all functions. We are not clearing the t_write thread and we are popping and causing a crash. Additionally modify OSPF_ISM_WRITE_ON(O) to not just blindly turn on the t_write thread. Only do so if we have data. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> ospfd: Remove redundant asserts assert(oi) is impossible all listgetdata(node) directly proceeding it already asserts here, besides a node cannot be created with a null pointer! If list_isempty is called directly before the listhead call it is impossilbe that we do not have a valid pointer here. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
This looks like a finish up of the partial cleanup that ocurred at some point in time in the past. When we alloc oi also always alloc the oi->obuf. When we delete oi always delete the oi->obuf right before. This cleans up a bunch of code to be simpler and hopefully easier to follow. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
donaldsharp
force-pushed
the
ospf_write_q
branch
from
275e0ed to
36a106e
Compare
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-8750/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am rarely seeing this crash:
r2: ospfd crashed. Core file found - Backtrace follows:
[New LWP 32748]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/aarch64-linux-gnu/libthread_db.so.1".
Core was generated by `/usr/lib/frr/ospfd'.
Program terminated with signal SIGABRT, Aborted.
2019-08-29 15:59:36,149 ERROR: assert failed at "test_ospf_sr_topo1/test_memory_leak":
Which translates to this code:
So if we get into ospf_write without anything on the oi_write_q
we are stopping the program.
The only place that I see where we could possibly have this condition
was in OSPF_ISM_WRITE_ON(O). Modify the macro to not just blindly
turn on ospf_write since we do not always add to the oi_write_q.
Signed-off-by: Donald Sharp sharpd@cumulusnetworks.com