fix: Avoid using strncat and a trailing newline in logs#301
Conversation
jan-auer
left a comment
There was a problem hiding this comment.
This is coming a bit late, probably, but: Why don't you just print in three stages? Something like:
fprintf(stderr, "[sentry] %s", sentry__logger_describe(level));
vfprintf(stderr, message, args);
fputc('\n', stderr);You have a few (well optimizable) function calls vs a string allocation here. The only disadvantage that I can see is that stderr can be unbuffered, but I think even that should not be an issue here.
It also could lead to messed output if application thread is also trying to write to the stderr at the same moment. |
That’s what I wanted to say. Even our example itself is quite spammy with the http thread in the background, and you do have interspersed logging there, which is quite difficult to follow as it is. Either way, while the allocation is suboptimal, I suspect people won’t have logging on in production anyway, and if they do they will probably integrate with their own system. |
|
Excellent point, I knew something was up with this :) Alright, let me take another stab at this. How about: const char *prefix = "[sentry] ";
const char *priority = sentry__logger_describe(level);
size_t total_len = strlen(prefix) + strlen(priority) + strlen(message) + 1;
char *format = sentry_malloc(total_len);
snprintf("%s%s%s", total_len, prefix, priority, message);
vfprintf(stderr, format, args);Of course, prefix could also be inlined, but that's probably more prone to errors when changing the prefix. |
|
I also thought about that before, but yes, lets do it ;-) |
jan-auer
left a comment
There was a problem hiding this comment.
Thanks for bearing with me :) I have to admit that I did not look into the performance implications of using snprintf over memcpy. My assumption is that this will still be optimized.
PR Title needs updating. Then G2G.
And avoid a trailing newline for custom loggers. fixes getsentry#298
The usage of
strcatbothers gcc-10 and clang analyzer.fixes #298