[systemd-devel] [PATCH 3/6] Optionally save core dumps as plain files

[Lennart Poettering]

On Tue, 12.02.13 01:14, Oleksii Shevchuk (alxchk at gmail.com) wrote:

> Save core dumps to /var/log/journal/MACHINE-ID/coredump/COMM.CORE-ID.
> If no access rights or directory not present, save core to journal.

Hmm, if we do this, then I'd suggest not to place this in the journal
directory. This should be independent of it. Note that the journal logic
currently watches the entire per-machine dir for changes and we thus
should really drop stuff there that doesn't belong there..

So, I'd propose using something like /var/lib/coredump/ or so, and then
also setup up tmpfiles to automatically clean this up, by time.

> -static int divert_coredump(void) {
> -        _cleanup_fclose_ FILE *f = NULL;
> +static int submit_process_core(struct iovec iovec[15], int idx,
> +                               const char * comm,
> +                               const int journal)
> +{

"const int" makes little sense. Also, booleans are "bool" in our sources (C99).

Please follow the coding style here, i.e. { on the same line as the function.


> +        int r = EXIT_FAILURE;
> +
> +        _cleanup_fclose_ FILE * corefile = NULL;
> +        _cleanup_free_   char * corepath = NULL;
> +        _cleanup_free_   char * corelink = NULL;
> +        _cleanup_free_   char * t = NULL;

Please follow coding style, no spurious spaces here, please...

> +
> +        if (journal) {
> +                mkdir_p_label("/var/lib/systemd/coredump", 0755);
> +                corelink = strdup("/var/lib/systemd/coredump/core.systemd-journald");
> +                if (! corelink) {
> +                        r = log_oom();
> +                        goto finish;
> +                }
> +
> +                corepath = strdup(corelink);
> +                if (! corepath) {
> +                        r = log_oom();
> +                        goto finish;
> +                }
> +        } else {
> +                _cleanup_free_ char * c;
> +
> +                char buffer[33];
> +                sd_id128_t coreid;
> +                sd_id128_t machineid;
> +
> +                const char *p = strrchr(comm, '/');
> +                if (p)
> +                        p ++;
> +                else
> +                        p = comm;

Hmm, so the comm field of the kernel usually is already the base name,
with the path stripped of. That said, applications can freely set the
comm field via PR_SET_NAME, so we really need to clean this up beofre
using this in the filename. Running xescape() on this should probably
already suffice...

> +
> +                r = sd_id128_get_machine(&machineid);
> +                if (r)
> +                        goto journal;
> +
> +                c = sd_id128_to_string(machineid, buffer);
> +                c = strjoin("/var/log/journal/", c, "/coredump", NULL);
> +                if (! c)
> +                        goto journal;
> +
> +                r = access(c, X_OK | W_OK);
> +                if (r)
> +                        goto journal;

This sounds racy... I'd always recommend just trying to open the file.

In general however, I'd like to see a proper configuration file
introduced for this:

/etc/systemd/coredump.conf

With booleans:

StoreInJournal=true/false
StoreInDirectory=true/false

and then enable the storing in /var exactly when the latter bool is set
(possibly even creating the directory to store things in.

(That file should later on then also learn a setting for enforcing a
user-configurable max coredump size, instead of the current compiled-in
value...)

> +
> +                r = sd_id128_randomize(&coreid);
> +                if (r)
> +                        goto journal;

Do we really want a new random ID in this?

> +
> +                corelink = strjoin(p, ".", sd_id128_to_string(coreid, buffer), NULL);
> +                if (! corelink)
> +                        goto journal;
> +
> +                corepath = strjoin(c, "/", corelink, NULL);
> +                if (! corepath)
> +                        goto journal;
> +        }
> +
> +        corefile = fopen(corepath, "w");

Not that it would matter here, but it's a good habit to always use "we"
rather than "w" in fopen...

> +
> +journal:
> +        if (! corefile) {
> +                if (journal) {
> +                        log_error("Failed to create coredump file: %m");
> +                        goto finish;
> +                }
> +                else {

Please write:

} else {


> +                        int n;
> +
> +                        IOVEC_SET_STRING(iovec[idx++], "MESSAGE_ID=fc2e22bc6ee647b6b90729ab34a250b1");
>  
> -        log_info("Detected coredump of the journal daemon itself, diverting coredump to /var/lib/systemd/coredump/.");
> +                        t = malloc(9 + COREDUMP_MAX);
> +                        if (!t) {
> +                                r = log_oom();
> +                                goto finish;
> +                        }
> +
> +                        memcpy(t, "COREDUMP=", 9);
>  
> -        mkdir_p_label("/var/lib/systemd/coredump", 0755);
> +                        n = loop_read(STDIN_FILENO, t + 9, COREDUMP_MAX, false);
> +                        if (n < 0) {
> +                                log_error("Failed to read core dump data: %s", strerror(-n));
> +                                r = (int) n;
> +                                goto finish;
> +                        }
>  
> -        f = fopen("/var/lib/systemd/coredump/core.systemd-journald", "we");
> -        if (!f) {
> -                log_error("Failed to create coredump file: %m");
> -                return -errno;
> +                        iovec[idx].iov_base = t;
> +                        iovec[idx].iov_len = 9 + n;
> +                        idx ++;
> +                }
>          }
> +        else {
> +                r = chmod(corepath, 0600);
> +                if (r) {
> +                        log_debug("chmod %s: %s", corepath, strerror(errno));
> +                }

Nah, it's always nicer to use fchmod(fileno(coreful)) for thigns like
that. But to make this fully correct it's probably better to use umask()
around the fopen(), so that the access mode is always set correctly.


> +        int journal = 0;

"bool" please. and "false".

Also, message IDs are *not* supposed to be "just counted up". Instead
generate a new one with "journalctl --new-id128". However, in this case
we really should just use the same ID for all cases, and just depending
on the configuration have a different set of keys in it, i.e. either the
actual coredump, or the coredump path + hash, or even both. Depending on
the configuration file...

Hope these comments make some sense.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.