On Fri, Jun 6, 2014 at 4:09 PM, Lennart Poettering <[email protected]> wrote: > On Sat, 31.05.14 23:29, Thomas H.P. Andersen ([email protected]) 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 _______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
