Bug 11154 - A patch to harden ccache logging in case of write failure, etc.
A patch to harden ccache logging in case of write failure, etc.
Status: CLOSED FIXED
Product: ccache
Classification: Unclassified
Component: ccache
dev
All All
: P5 normal
: 3.2.2
Assigned To: Joel Rosdahl
Joel Rosdahl
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-03-11 15:40 UTC by ishikawa
Modified: 2015-05-10 12:32 UTC (History)
1 user (show)

See Also:


Attachments
Harden logging by checking I/O error, etc. (2.74 KB, patch)
2015-03-11 15:40 UTC, ishikawa
no flags Details
Attachments Harden logging by checking I/O error, etc. (Take 2) (2.79 KB, patch)
2015-03-18 15:24 UTC, ishikawa
no flags Details
Attachments Harden logging by checking I/O error, etc. (Take 3) (2.73 KB, patch)
2015-03-19 14:52 UTC, ishikawa
ishikawa: review? (joel)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description ishikawa 2015-03-11 15:40:07 UTC
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
Comment 1 Joel Rosdahl 2015-03-14 15:00:08 UTC
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.
Comment 2 ishikawa 2015-03-17 06:22:37 UTC
(In reply to Joel Rosdahl from comment #1)

Thank you for your comment.
I will prepare a new patch based on your comment.

TIA
Comment 3 ishikawa 2015-03-18 15:24:46 UTC
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
Comment 4 Joel Rosdahl 2015-03-18 18:57:09 UTC
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?
Comment 5 ishikawa 2015-03-19 11:40:25 UTC
(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
Comment 6 ishikawa 2015-03-19 14:52:11 UTC
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
Comment 7 Joel Rosdahl 2015-03-19 20:30:14 UTC
Applied, thanks!
Comment 8 Joel Rosdahl 2015-05-10 12:32:59 UTC
Included in v3.2.2.