[systemd-devel] [PATCH v4 1/2] Move handling of sysv initscripts to a generator

[Lennart Poettering]

On Sat, 31.05.14 23:29, Thomas H.P. Andersen (phomes at gmail.com) wrote:

Thanks! Awesome work! I am enjoying this!

A few comments:

> +
> +static int generate_unit_file(SysvStub *s) {
> +        char *unit;
> +        char **p;
> +        _cleanup_fclose_ FILE *f = NULL;
> +        _cleanup_free_ char *before = NULL;
> +        _cleanup_free_ char *after = NULL;
> +        _cleanup_free_ char *conflicts = NULL;
> +        int r;
> +
> +        before = strv_join(s->before, " ");
> +        after = strv_join(s->after, " ");
> +        conflicts = strv_join(s->conflicts, " ");

misses OOM checks!

> +
> +        unit = strjoin(arg_dest, "/", s->name, NULL);
> +        if (!unit)
> +                return log_oom();
> +
> +        f = fopen(unit, "wxe");
> +        if (!f) {
> +                log_error("Failed to create unit file %s: %m", unit);
> +                return -errno;
> +        }
> +
> +        fprintf(f,
> +                "# Automatically generated by systemd-sysv-generator\n\n"
> +                "[Unit]\n"
> +                "SourcePath=%s\n"
> +                "Description=%s\n",
> +                s->path, s->description);
> +
> +        if (!isempty(before))
> +                fprintf(f, "Before=%s\n", before);
> +        if (!isempty(after))
> +                fprintf(f, "After=%s\n", after);
> +        if (!isempty(conflicts))
> +                fprintf(f, "Conflicts=%s\n", conflicts);
> +
> +        fprintf(f,
> +                "\n[Service]\n"
> +                "Type=forking\n"
> +                "Restart=no\n"
> +                "TimeoutSec=5min\n"
> +                "IgnoreSIGPIPE=no\n"
> +                "KillMode=process\n"
> +                "GuessMainPID=no\n"
> +                "RemainAfterExit=%s\n",
> +                s->pid_file ? "no" : "yes");

Maybe use "yes_no(!s->pid_file)" for this? it's a macro that does
exactly this ternary operator check.

> +
> +        if (s->sysv_start_priority > 0)
> +                fprintf(f, "SysVStartPriority=%d\n", s->sysv_start_priority);
> +
> +         if (s->pid_file)
> +                 fprintf(f, "PidFile=%s\n", s->pid_file);
> +
> +         fprintf(f, "ExecStart=%s start\n", s->path);
> +         fprintf(f, "ExecStop=%s stop\n", s->path);

Please merge these two lines in one fprintf() invocation, like we do it
for the other cases...

> +         if (s->reload)
> +                 fprintf(f, "ExecReload=%s reload\n", s->path);
> +
> +        STRV_FOREACH(p, s->wants) {

Looks like a whitespace issue... Only spaces please.

> +                        service = new0(SysvStub, 1);
> +                        if (!service)
> +                                return log_oom();
> +
> +                        service->sysv_start_priority = -1;
> +                        service->name = name;
> +                        service->path = fpath;
> +
> +                        hashmap_put(all_services, service->name,
> service);

This can fail, due to OOM, we need to guard for this...


> +        r = lookup_paths_init(
> +                        &lp, SYSTEMD_SYSTEM, true,
> +                        NULL, NULL, NULL, NULL);

Instead of making this a global variable, I'd prefer we could just pass
that through as arguments to the functions that need this.

Looks good otherwise!

Thanks!

Lennart

-- 
Lennart Poettering, Red Hat