On Tue, 12.02.13 01:14, Oleksii Shevchuk (alx...@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. _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel