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

[Thomas H.P. Andersen]

On Fri, Jun 6, 2014 at 4:09 PM, Lennart Poettering
<lennart at poettering.net> wrote:
> 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. Pushed with fixes.

> Thanks!
>
> Lennart
>
> --
> Lennart Poettering, Red Hat