runmodes: suricata hint missing runmode -v1 #9674
Conversation
|
NOTE: This PR may contain new authors: |
@comfort619 Can you please keep the author name as you have for your other merged contribution? It was Comfort Amaechi. This helps our checks know that you're not actually a new author ;) |
There was a problem hiding this comment.
Thanks for addressing this!
Some changes needed, from my end.
As this repo is for Suricata, we don't need to mention it in the commit message. Something like
runmodes: clear hint when runmode is not indicated
would work, imho.
Please also check the inline comments.
I've left some considerations about the added unit test, which I don't think we'll need.
| #include "suricata-common.h" | ||
| #include "util-running-modes.h" | ||
| #include "util-conf.h" | ||
| #include "runmodes.h" | ||
| #include "util-unittest.h" | ||
|
|
||
| void test_missing_run_mode(void) { | ||
| // Simulate missing run mode and check error message | ||
| SCInstance suri; | ||
| char *argv[] = {"suricata", NULL}; | ||
|
|
||
| int result = FinalizeRunMode(&suri, argv); | ||
|
|
||
| FAIL_IF(result != TM_ECODE_FAILED); | ||
| // Add checks to verify the error message, if possible | ||
| PASS; | ||
| } | ||
|
|
||
| UtRegisterTest("RunModeTest", test_missing_run_mode); |
There was a problem hiding this comment.
Considering you've created a Suricata-verify test for this addition, I think we don't need a unit test for it, but thanks for thinking about this! :)
There was a problem hiding this comment.
If we were to add this test and a new file, we'd need to:
- ensure we have added the Copyright note, adjusting it to the current year (you'll see it in basically all of our code files, for instance, check src/tests/detect-snmp-community.c )
- In case we did not have any unittests for this module, you would need to declare a new function following the convention
ModuleRegisterTests, in which you would add theUtRegisterTestcall that you have on line 19. - add the function that registers the RunMode unittest to the
runmodes-unittests.cfile, to ensure that your unittests is indeed run when Suricata runs the unittests. Check https://github.com/OISF/suricata/blob/master/src/runmode-unittests.c#L130 - adapt the unittest name to our naming conventions for C. We use camel case, as you'll notice from the other functions on that file ;)
There was a problem hiding this comment.
Okay, thank you for the feedback. Should I go ahead and make the changes for the unit test considering you indicated the probability of it being added
There was a problem hiding this comment.
No, I really don't think we'll need them in this case, I just gave the feedback as extra info that can be useful and expanded to any other similar contributions. :)
I would say the best path here is to remove the unittest and focus on having a good SV test.
| @@ -2369,6 +2369,7 @@ static int FinalizeRunMode(SCInstance *suri, char **argv) | |||
| { | |||
| switch (suri->run_mode) { | |||
| case RUNMODE_UNKNOWN: | |||
| SCLogError("Error: Missing run mode. Please specify a capture runmode."); | |||
There was a problem hiding this comment.
nit: there's no need to use Error in the message itself, for the usage of SCLogError will ensure that the log output will indicate that this is an error message.
Ticket: 5711
Link to redmine ticket:
Describe changes:
-Suricata when run does not provide feedback when the runmode is missing, leading to confusion for users
-Added missing runmode error
SV_BRANCH=OISF/suricata-verify#1434