On Tue, Sep 1, 2015 at 1:04 AM, FX wrote: >> the attached patch improves the error handling for backtrace failing, >> by printing the error number or the error string in addition to the >> message. It also fixes a potential null pointer crash in gf_strerror. >> >> Regtested on x86_64-pc-linux-gnu, Ok for trunk? > > Mostly OK. I have one question, though: in the specific case from the PR, the error in the backtrace is an OS error (allocating memory), which had set the errno variable. But in other cases, it is (I think) possible that libbacktrace itself fails and call error_callback() without errno being set. In that case, the errno message will be confusing. Hmm, in that case errnum must be set to 0. What about the attached patch, which prints the existing message if errnum == 0, and the new and improved only for errnum > 0? > > So, for that part of the patch, I think it would make more sense to change libbacktrace to give out a reasonable error message (and not “mmap”). A quick grep through the libbacktrace sources reveal that most calls to error_callback() actually provide insightful error messages. The ones that need to be improved are: > > alloc.c: error_callback (data, "malloc", errno); > alloc.c: error_callback (data, "realloc", errno); > alloc.c: error_callback (data, "realloc", errno); > mmap.c: error_callback (data, "mmap", errno); > mmapio.c: error_callback (data, "mmap", errno); > mmapio.c: error_callback (data, "munmap", errno); > posix.c: error_callback (data, "close", errno); > read.c: error_callback (data, "lseek", errno); > read.c: error_callback (data, "read", errno); This seems to be the unix tradition of terse error messages, I suppose it depends on other libbacktrace users whether such a change would be seen as desirable.. -- Janne Blomqvist