[systemd-devel] [PATCH] [RFCv3] Optionally save core dumps as plain files

[Lennart Poettering]

On Fri, 01.03.13 16:24, Oleksii Shevchuk (alxchk at gmail.com) wrote:

Heya! Sorry for the late review!

> +        <refnamediv>
> +                <refname>coredump.conf</refname>
> +                <refpurpose>Coredump utility configuration
> file</refpurpose>

We probably should spell this "core dump" rather than "coredump".

> +
> +                                <listitem><para>Enforce UID/GID of
> +                                <literal>systemd-coredump</literal> while loading
> +                                and storing coredump. If not setted, then uid/gid of
> +                                crashed process will be used.
> +                                </para></listitem>
> +                        </varlistentry>

s/setted/set/.

This explanation doesn't make clear why one would want to set this,
i.e. what the real effect of this is.

Also, we should minimize the options we have, and only add options that
really make sense, what's the use case for this one?

> +
> +                        <varlistentry>
> +                                <term><varname>SaveToJournal=</varname></term>
> +
> +                                <listitem><para>Tales a boolean value. If enabled
> +                                (the default) core dump will be stored in the
> +                                COREDUMP= field of log message.
> +                                </para></listitem>
> +                        </varlistentry>

s/Tales/Takes/.

> +
> +                        <varlistentry>
> +                                <term><varname>SaveToFilesystem=</varname></term>
> +
> +                                <listitem><para>One of
> +                                <literal>volatile</literal>,
> +                                <literal>persistent</literal> and
> +                                <literal>none</literal>.
> +                                value. If <literal>volatile</literal>, core dump will be
> +                                stored as file to /run/log/coredump/MACHINE-ID/COMM-TMPSUFFIX location;
> +                                if <literal>persistent</literal> -- to
> +                                /var/log/coredump/MACHINE-ID/COMM-TMPSUFFIX location.
> +                                Storage directory will be created if not exists. If
> +                                <literal>none</literal>, then core dump will not be stored
> +                                as file at all.
> +                                <literal>User=</literal> and <literal>Group=</literal>
> +                                not setted, then directory will be created with 1777
> +                                permissions, if not exists. In other case, directory
> +                                will be created with 0770 permissions, and with specified
> +                                user/group owner. If only one of <literal>User=</literal>,
> +                                <literal>Group=</literal> specified, then missing owner
> +                                will be root. Field COREDUMP_FILE= will be added to log
> +                                message with COMM-TMPSUFFIX value.</para></listitem>
> +                        </varlistentry>

I am not convinced we should create a new directory for this in
/var/log. I'd prefer to use /var/lib/systemd/coredump/ instead,
i.e. where we already put journald crashes.

Unless we have a really good reason to we really should keep things
under our own namespace...

> +#MaxFileCoreSize=4294967296

Why precisely this value? I mean, this sounds pretty arbitrary,
especially on 64bit archs... But if it is arbitrary we probably should
use something more humanly understandable, such as 2G or so.

> +
> +  You should have received a copy of the GNU Lesser General Public License
> +  along with systemd; If not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +#include "conf-parser.h"
> +
> +#define COREDUMP_MESSAGEID "fc2e22bc6ee647b6b90729ab34a250b1"

Hmmm? Why this?

SD_ID128_CONST_STR(SD_MESSAGE_COREDUMP) looks like the more obvious
choice...

> +#define COREDUMP_PERSISTENT_DIRECTORY "/var/log/coredump"
> +#define COREDUMP_VOLATILE_DIRECTORY "/run/log/coredump"

We actually usually just hardcode these directoes, rather making things
unreadable via indirections unless there is a really good reason to make
them configurable...

> +#define COREDUMP_CORE_LABEL "COREDUMP"
> +#define COREDUMP_FILE_LABEL "COREDUMP_FILE"
> +#define COREDUMP_CORE_RECORD COREDUMP_CORE_LABEL "="
> +#define COREDUMP_FILE_RECORD COREDUMP_FILE_LABEL "="
> +#define COREDUMP_CORE_RECORD_SIZE sizeof(COREDUMP_CORE_RECORD)
> +#define COREDUMP_FILE_RECORD_SIZE sizeof(COREDUMP_FILE_RECORD)

I am not convinced we should have defines for this in the headers...

Otherwise looks pretty OK!

Sorry again for the long long delay in reviewing!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.