Remove assertions and skip on missing or broken rrname, bailiwick, rdata on nmsg files#24
Remove assertions and skip on missing or broken rrname, bailiwick, rdata on nmsg files#24regalk13 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
I would suggest changing this to
if (((count_messages + count_skipped) % STATS_INTERVAL) == 0)
but otherwise, everything looks good
There was a problem hiding this comment.
Although I'm on board with replacing these assert() calls, this change risks silently discarding indicators of a potentially serious upstream issue.
Preferable alternatives would be:
- exit with a failure code rather than
abort()on bad data, - count bad data, and exit with a failure code if
count_skipped > 0, or - option 1, but with logging.
I would be open to making "skip and count invalid messages" a configurable non-default behavior.
I vote for option #3 with the option to skip+count invalid messages. |
045b795 to
89a3e63
Compare
|
|
||
| for (;;) { | ||
| int32_t vid, msgtype; | ||
| bool some_skipped; |
There was a problem hiding this comment.
This should be initialized -- C does not automatically initialize variables, so this could be true / nonzero leading to improperly skipped data.
| if (some_skipped) { | ||
| nmsg_message_destroy(&msg); | ||
| count_skipped += 1; | ||
| continue; |
There was a problem hiding this comment.
The default behavior should be to exit cleanly on invalid input, or break; here, so we don't continue processing when we've decided the input is not 100% valid data.
| ubuf_destroy(&val); | ||
| do_stats(); | ||
|
|
||
| if (count_skipped >= 1) |
There was a problem hiding this comment.
From a style perspective, count_skipped > 0 is preferable to count_skipped >= 1 here.
Replaces fatal assert() crashes on invalid records (missing or empty rrname/bailiwick/...) with per-record skip so conversion continues past bad data. And add count_skipped counter reported alongside count_messages.