-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Updated Code from PR #2304 #3371
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
Conversation
This code was written by: @brandonpayton Co-authored-by: @brandonpayton
Signed-off-by: Kasumi <kasumi@kitsune.exposed>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Sonarcloud shows here you should use nullptr instead of NULL. I see there are many other NULL literals in this file - feel free to replace them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is a completely valid solution, but as you can see in other "C" API functions, the authors tied to avoid all the logic and use C++ types (like here the bool and std::string).
My suggestion here: create a thin wrapper, an "extra" function.
You should add a new function to RulesSet class with name reopen(), move this body there, and in this "C" API you have to call only that function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally: please don't use this file to collect suppressions. If you need to add some suppression, please use an inline one.
But here: why it this necessary? It seems in that file in line 60 there is only a condition...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here, but it seems that file only has 77 lines, so adding line 88 makes no sense.
Hi @akama-aka,
many thanks for this pull request, and special welcome to your first contribution!
I added some questions above, please review them.
And one more question - you wrote:
Implements the ability to reopen audit log files to ensure compatibility with Linux log rotation
Could you try this patch on Windows? I think the result is almost the same, but I'm curios how Windows handles the reopen() method. Do you have any idea how can we add a test case to check this method?
Cc: @eduar-hte (regarding to Windows).
And finally: I saw you added a "C" API function. I assume that will be used in Nginx to reopen audit.log files, right? Do you mind to add that feature to Nginx connector if we merge this?
Hi @akama-aka,
many thanks for this pull request, and special welcome to your first contribution!
I added some questions above, please review them.
And one more question - you wrote:
Implements the ability to reopen audit log files to ensure compatibility with Linux log rotation
Could you try this patch on Windows? I think the result is almost the same, but I'm curios how Windows handles the
reopen()method. Do you have any idea how can we add a test case to check this method?Cc: @eduar-hte (regarding to Windows).
And finally: I saw you added a "C" API function. I assume that will be used in Nginx to reopen audit.log files, right? Do you mind to add that feature to Nginx connector if we merge this?
Thank you for your comment to this draft pr. I'll try to learn C/C++ more to understand more of it and try to fix those things up ^^
Quality Gate Passed Quality Gate passed
Issues
4 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
Uh oh!
There was an error while loading. Please reload this page.
I won't continue that because I don't have the time and motivation to do so. Even tho it's just vibe coded and thats not a way how to solve issues or missing features.