Introduce logging functionality
Hello @adsr
Another PR from me. Please take a look when you have time :)
So currently there's no logging in termbox2 at all. I'd like to propose a simple implementation of logging functionality into the library to make it easier to maintain. Just to justify introduction of this functionality, here are my points regarding usual argument of bloat vs utility in this case:
-
when user of application based on termbox2 reports a bug with interface failing to initialize, all what I can do as a maintainer of application is to ask them to debug termbox2 code on their side themselves via either debugger or manual printf logging (case study: https://codeberg.org/newsraft/newsraft/issues/212). This is not nice as it raises the required effort and time for bug reporting, which makes the user not want to reveal the problem fully, with a risk of losing sight of potential bugs that users did not want to spend too much of their time on;
-
there are some places in the source code of termbox2 where errors can happen but they're not handled because their handling is too tedious and their possibility of occurrence is so negligible small that it's considered to never fail in this case anyways. But it still essential to at least have information about abnormal states like this. For example:
@@ -3464,7 +3474,9 @@ static int wait_event(struct tb_event *event, int timeout) {
if (resize_has_events) {
int ignore = 0;
- read(global.resize_pipefd[0], &ignore, sizeof(ignore));
+ if (read(global.resize_pipefd[0], &ignore, sizeof(ignore)) < 0) {
+ tb_log("Couldn't read data from resize pipe even though file descriptor was set!");
+ }
// TODO: Harden against errors encountered mid-resize
if_err_return(rv, update_term_size());
if_err_return(rv, resize_cellbufs());
@@ -3775,7 +3787,9 @@ static int resize_cellbufs(void) {
static void handle_resize(int sig) {
int errno_copy = errno;
- write(global.resize_pipefd[1], &sig, sizeof(sig));
+ if (write(global.resize_pipefd[1], &sig, sizeof(sig)) < 0) {
+ tb_log("Couldn't write to resize pipe to notify about resize signal!");
+ }
errno = errno_copy;
}
By the way, currently these unhandled calls to read and write yield a warning when compiled with strict rules:
src/termbox2.h: In function ‘handle_resize’:
src/termbox2.h:3057:5: warning: ignoring return value of ‘write’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
3057 | write(global.resize_pipefd[1], &sig, sizeof(sig));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/termbox2.h: In function ‘wait_event’:
src/termbox2.h:2743:13: warning: ignoring return value of ‘read’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
2743 | read(global.resize_pipefd[0], &ignore, sizeof(ignore));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- some places in code can be commented with log messages instead of comments. That obviously is more useful as it makes our precious context information be visible not just when reading the code but also when reading the logs after the incident of bug occurrence. For example:
@@ -3410,8 +3410,7 @@ static const char *get_terminfo_string(int16_t offsets_pos, int16_t offsets_len,
int str_offset = (int)table_pos + (int)table_offset;
if (str_offset >= (int)global.nterminfo) {
- // string beyond end of terminfo entry
- // Truncated/corrupt terminfo entry?
+ tb_log("Offset %d is beyond end of terminfo data. Is terminfo corrupted?", str_offset);
return NULL;
}
- it's just nice to have as much information about processes going on in termbox2 as possible when you go through another bug report. It's good to know that some subsystem of your application worked fine and there's a log message about it. This reduces the level of uncertainty when analyzing reports about bugs in your application and makes the maintenance of the application based on termbox2 much faster and easier (see point 1). Just look at this pretty:
[TERMBOX] Couldn't find term screen in terminfo database ./newsraft-test-terminfo
[TERMBOX] Couldn't find term screen in terminfo database /home/txgk/.terminfo
[TERMBOX] Couldn't find term screen in terminfo database /usr/local/etc/terminfo
[TERMBOX] Couldn't find term screen in terminfo database /usr/local/share/terminfo
[TERMBOX] Couldn't find term screen in terminfo database /usr/local/lib/terminfo
[TERMBOX] Couldn't find term screen in terminfo database /etc/terminfo
[TERMBOX] Successfully read terminfo from /usr/share/terminfo/s/screen (1607 bytes)
I most certainly missed some places worthy of logging with tb_log() but I believe it's best to first have the functionality introduced and then iteratively arrange log statements around the codebase based on the practical criticality of particular places in code. I'm open to suggestions where you'd also want to put tb_log() calls.
The patch I propose here is already present in Newsraft and works wonderful. Here's how it's used: https://codeberg.org/newsraft/newsraft/src/commit/fd4c58b9436d3f081c9c79025ebcc613dc2fc29b/src/interface.c#L44
Best regards, Grigory