Datasets locking/v5 #12334
Datasets locking/v5 #12334
Conversation
In a recent warning reported by scan-build, datasets were found to be
using a blocking call in a critical section.
datasets.c:187:12: warning: Call to blocking function 'fgets' inside of critical section [unix.BlockInCriticalSection]
187 | while (fgets(line, (int)sizeof(line), fp) != NULL) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
datasets.c:292:12: warning: Call to blocking function 'fgets' inside of critical section [unix.BlockInCriticalSection]
292 | while (fgets(line, (int)sizeof(line), fp) != NULL) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
datasets.c:368:12: warning: Call to blocking function 'fgets' inside of critical section [unix.BlockInCriticalSection]
368 | while (fgets(line, (int)sizeof(line), fp) != NULL) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
datasets.c:442:12: warning: Call to blocking function 'fgets' inside of critical section [unix.BlockInCriticalSection]
442 | while (fgets(line, (int)sizeof(line), fp) != NULL) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
datasets.c:512:12: warning: Call to blocking function 'fgets' inside of critical section [unix.BlockInCriticalSection]
512 | while (fgets(line, (int)sizeof(line), fp) != NULL) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
5 warnings generated.
These calls are blocking in the multi tenant mode where several tenants
may be trying to load the same dataset in parallel.
In a single tenant mode, this operation is performed as a part of a
single thread before the engine startup.
In order to evade the warning and simplify the code, the initial file
reading is moved to Rust with this commit with a much simpler handling
of dataset and datarep.
Bug 7398
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12334 +/- ##
==========================================
- Coverage 83.26% 83.24% -0.02%
==========================================
Files 912 913 +1
Lines 257643 257651 +8
==========================================
- Hits 214521 214479 -42
- Misses 43122 43172 +50
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 24062 |
What is the reason for doing this change as part of these commits? Seems like a behavior change should have its own commit? |
|
It is used to for errors to be fatal on initial startup, but not be fatal during a rule reload. We need it to stay as is. I think the simplest think to do would be to have a C helper func that you can expose to Rust, hiding the implementation details like an atomic. |
Thank you very much. Hadn't considered the reloads. 🙇🏽♀️ |
Link to ticket: https://redmine.openinfosecfoundation.org/issues/7398
Previous PR: #12326
Changes since v4:
Note: This introduces a change in behavior for the case when one or more datasets read from a specific file were not added to the hash for some reason.
init-errors-fatalinit-errors-fatalQ: Wdyt about this change in behavior? I do not understand why we'd want to allow adding partial datasets from a file..