On Thu, 12.07.12 16:00, Michal Sekletar ([email protected]) 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.
_______________________________________________
systemd-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
