Created attachment 10855 [details] Harden logging by checking I/O error, etc. CAVEAT: The uploaded patch was created during the work to support -gsplit-dwarf (FissionDwarf) of GCC. (See bug 10005 ) Some of the patches have interdependence and if earlier patches are not applied, the later ones may fail. I will re-create patches appropriately depending on how an earlier patch is incorporated (or dropped). The whole list of broken patches below were uploaded in bug 10005 for convenience, but now separate bugzilla is filed to follow up on each of it (except for the main -gsplit-dwarf support, in bug 10005). *0001-Harden-logging-perror-replaced-with-cc_log-and-fatal.patch 0002-hash-file-creation-failure-is-recorded-now.patch 0003-Use-x_unlink-instead-of-unlink-per-HACKING.txt.patch 0004-make-sure-the-result-of-stat-call-is-checked.patch 0005-Enhance-log-message-add-prefix-var.-name-and-a-few-a.patch 0006-warning-about-missing-x_unlinks-when-get_file_from_c.patch 0007-Support-for-gsplit-dwarf.patch 0008-Missing-copy-of-diagnostics-file-in-case-of-failure.patch What this entry is about: Short explanation. 1: 0001-Harden-logging-perror-replaced-with-cc_log-and-fatal.patch harden logging: trace write errors and/or abort perror() -> cc_fail and fatal This was necessary when I realized that log was not written to a smallish partition for testing and the partition filled up. In the patch, the error code from low-level output routine is checked and error action is taken (basically, it tries to abort now). Also, use of perror() should not be done but rather the sequence of cc_log() and fatal() is better according to a message by Joel in bug 10005. This is to util.c only and so can be independent of main ccache.c change to support -gsplit-dwarf. (see bug 10005) This patch is actually not related to -gsplit-dwarf and I so I am filing this separate bugzilla here. TIA
Thanks! Some comments on the patch: > +/* > + * warn the failure of logging function. > + * exit then. Perhaps reformulate as "Warn about failure writing to the log file and then exit."? > + */ > +static void > +warn_log_fail() { Minor detail, but still: Put curly brace on new line. > + fprintf(stderr, > + "Writing to logfile failed.\n" > + "Check the permission and make sure there is enough room for writing to the filesystem.\n"); I suggest mimicking the output from fatal instead and include strerror(errno), i.e. something like this: fprintf(stderr, "ccache: error: Failed to write to %s: %s", conf->logfile, strerror(errno)); > + /* Maybe we should exit here! */ Yes, because otherwise I guess fatal will call cc_log, which will call vlog, which will try to write to the file and then (assuming it still fails to write to the file) call fatal -> recursion? > if (errno != ENOENT) { > - perror(fname); > + cc_log("lstat on file %s failed.", fname); > + fatal("lstat on file %s failed.", fname); > } Good find! But it should be enough to call fatal() since it calls cc_log already. I suggest including strerror(errno) as well: fatal("lstat %s failed: %s", fname, strerror(errno)); > + int rc; > + cc_log("tmp_unlink:Unlink %s", path); I don't the "tmp_unlink:" prefix contributes with much. The reader of the ccache logfile is not expected to know about ccache's internal function names. > + cc_log("tmp_unlink:Unlink failed: rc = %d", rc); Better to write strerror(errno) than rc since rc is always -1 if it fails.
(In reply to Joel Rosdahl from comment #1) Thank you for your comment. I will prepare a new patch based on your comment. TIA
Created attachment 10880 [details] Attachments Harden logging by checking I/O error, etc. (Take 2) Hi, I created a new patch that incorporates your suggestions. I modified a few cc_log() messages that were not directly mentioned in your comment, and also in one case, saved errno so that it can be used to print strerrno() string in later cc_log() call. TIA
The patch uses space instead of tabs for indentation in some places. I can correct it when applying the patch if you like (if it's hard to get it right). > +warn_log_fail() Add missing "void" as the parameter list. > + fprintf(stderr, "ccache: error: Failed to write to %s: %s", > + conf->log_file, strerror(errno)); There needs to be a "\n" at the end of the format string (sorry for not including it in my sketch). > > + /* Maybe we should exit here! */ Did you see my previous comment regarding this? Won't there be endless recursion if the call to fatal is kept?
(In reply to Joel Rosdahl from comment #4) >The patch uses space instead of tabs for indentation in some places. I can >correct it when applying the patch if you like (if it's hard to get it right). OK I will be very careful next time :-) >> +warn_log_fail() > > Add missing "void" as the parameter list. will do. >> + fprintf(stderr, "ccache: error: Failed to write to %s: %s", >> + conf->log_file, strerror(errno)); >There needs to be a "\n" at the end of the format string (sorry for not >including it in my sketch). Will do. > > > + /* Maybe we should exit here! */ > Did you see my previous comment regarding this? Won't there be endless ? > recursion if the call to fatal is kept? Yes, I read it and understood the issue, but forgot to modify the code/comment. What about /* We should exit here now. */ or something to that effect as far as the comment is concerned. TIA
Created attachment 10890 [details] Attachments Harden logging by checking I/O error, etc. (Take 3) Hi, I believe this patch contains only tab for leading whitespace and modifications explained in your comments. Please feel free to change the comment about recursive stuff. I am not sure what the proper comment would be. TIA
Applied, thanks!
Included in v3.2.2.