On Mon, 06.08.12 16:45, Lukáš Nykrýn (lnyk...@redhat.com) wrote:
> + if (!*set) {
> + set_free(*set);
> + *set = NULL;
> + }
You probably mean if (*set) here, not (!*set). But honestly I think this
bit should just go entirely as for most other settings we actually do
allow adding in additional values later on if it makes sense (example:
Environment=).
> +
> + *set = set_new(trivial_hash_func, trivial_compare_func);
> + if (!*set)
> + return -ENOMEM;
We probably should start using log_oom() for things like this now that
it is available.
> +
> + FOREACH_WORD(w, l, rvalue, state)
> + {
> + char *temp = new0(char, l + 1);
> + if (!temp)
> + return -ENOMEM;
Please use strndup() here! (And also log_oom() here please).
> +
> + strncpy(temp, w, l);
> + r = safe_atoi(temp, &val);
> + free(temp);
> + if (r < 0) {
> + log_error("[%s:%u] Failed to parse numeric value:
> %s", filename, line, w);
> + set_free(*set);
> + *set = NULL;
I'd leave the hashmap set, as it is freed anyway when the service goes
away, and makes simpler and easier to read.
> + return r;
> + }
> + r = set_put(*set, INT_TO_PTR(val));
> + if (r < 0) {
> + log_error("[%s:%u] Unable to store: %s", filename,
> line, w);
> + set_free(*set);
> + *set = NULL;
Same here.
Otherwise looks good.
(We want the signal stuff eventually I guess, but we can easily add this later).
Lennart
--
Lennart Poettering - Red Hat, Inc.
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
