[systemd-devel] [PATCH] systemd: enable/disable instances of template

[Lennart Poettering]

On Thu, 12.07.12 16:00, Michal Sekletar (msekleta at redhat.com) wrote:

> +                                template = unit_name_template(info->name);
> +                                if (!template) {
> +                                        free(path);
> +                                        return -ENOMEM;
> +                                }
> +
> +                                if (isempty(root_dir))
> +                                        asprintf(&template_dir, "%s/", *p);
> +                                else
> +                                        asprintf(&template_dir, "%s/%s/", root_dir, *p);
> +
> +                                if (!template_dir) {
> +                                        free(path);
> +                                        free(template);
> +                                        return -ENOMEM;
> +                                }
> +
> +                                template_path = join(template_dir, template, NULL);
> +                                if (!template_path) {
> +                                        free(path);
> +                                        free(template);
> +                                        free(template_dir);
> +                                        return -ENOMEM;
> +                                }

AFAICS we only need template_path, never template_dir. Can't we just
concatenate the full string instead of template_dir first, and then
template_path from it?

> +                if (unit_name_is_instance(i->name)) {
> +                        char *template = NULL,
> +                             *instance_path = NULL;
> +
> +                        /* create path to instance */
> +                        instance_path = new0(char, strrchr(i->path, '/') - i->path + strlen(i->name) + 2);
> +                        memcpy(instance_path, i->path, strrchr(i->path, '/') - i->path + 1);
> +                        memcpy(instance_path + strlen(instance_path),
> i->name, strlen(i->name));


This code will run as part of PID 1, this *must* be OOM safe.

Also, strrchr() doesn't come for free, please run this once and save the
result. Also path_get_file_name() sounds like the better choice
here. Oh, and are you aware of the wonders of stpcpy()? It makes code
like this quite often much much nicer to read.

Otherwise looks good.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.