On Sat, 31.05.14 23:29, Thomas H.P. Andersen (pho...@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
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
