-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
MySQL: parser v2 #12541
base: master
Are you sure you want to change the base?
MySQL: parser v2 #12541
Conversation
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.
Just a quick scan on some naming and visibility of methods that we're trying to stick to with new code, and update existing code as needed. Probably more review comments to come though.
"description": "Mysql server version" | ||
}, | ||
"tls": { | ||
"type": "boolean" |
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.
Description please.
"description": "sql query statement or some utility commands like ping." | ||
}, | ||
"affected_rows": { | ||
"type": "integer" |
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.
Description please.
#[no_mangle] | ||
unsafe extern "C" fn SCMysqlCommandSetup( |
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.
No pub, and only used by function pointer, this doesn't have to be #[no_mangle]
, and can follow Rust naming convention.
} | ||
|
||
#[no_mangle] | ||
unsafe extern "C" fn SCMysqlGetCommand( |
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.
Again, no pub, probably doesn't need no_mangle.
); | ||
} | ||
|
||
#[no_mangle] |
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.
Again, no_mangle not needed.
#[no_mangle] | ||
pub unsafe extern "C" fn rs_mysql_state_get_tx( |
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.
Doesn't need no_mangle and pub. Also drop the rs_ prefix.
#[no_mangle] | ||
pub unsafe extern "C" fn rs_mysql_tx_get_alstate_progress( |
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.
Doesn't need no_mangle and pub. Also drop the rs_ prefix.
const PARSER_NAME: &[u8] = b"mysql\0"; | ||
|
||
#[no_mangle] | ||
pub unsafe extern "C" fn rs_mysql_register_parser() { |
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.
Naming, SCMysqlRegisterParser
.
use suricata_derive::EnumStringU8; | ||
|
||
#[allow(dead_code)] | ||
pub const CLIENT_LONG_PASSWORD: u32 = BIT_U32!(0); |
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.
Do these need to be pub?
payload: &'a mut [u8], | ||
} | ||
|
||
impl<'a> Drop for MysqlPacket<'a> { |
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.
Clippy says the lifetimes are not needed here.
NOTE: This PR may contain new authors. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12541 +/- ##
==========================================
- Coverage 80.68% 80.51% -0.18%
==========================================
Files 925 929 +4
Lines 258914 261234 +2320
==========================================
+ Hits 208914 210329 +1415
- Misses 50000 50905 +905
Flags with carried forward coverage won't be shown. Click here to find out more. |
Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
Link to ticket: https://redmine.openinfosecfoundation.org/issues/3446
Describe changes:
mysql.command
andmysql.rows
SV_BRANCH=OISF/suricata-verify#2067