[systemd-devel] [PATCH] core: reuse the same /tmp and /var/tmp

[Tom Gundersen]

On Fri, Feb 1, 2013 at 6:43 PM, Michal Sekletar <msekleta at redhat.com> wrote:
> All Execs within the service, will get mounted the same /tmp and /var/tmp
> directories, if service is configured with PrivateTmp=yes. Temporary
> directories are cleaned up by service itself, rather than relying on
> systemd-tmpfiles.
> ---

Great that you are picking this up, I have been wanting it for a long time.

> +void exec_context_tmpdirs_done(ExecContext *c) {
> +        if (c->tmp_dir) {
> +                rm_rf_dangerous(c->tmp_dir, false, true, false);
> +                free(c->tmp_dir);
> +                c->tmp_dir = NULL;
> +        }
> +
> +        if (c->var_tmp_dir) {
> +                rm_rf_dangerous(c->var_tmp_dir, false, true, false);
> +                free(c->tmp_dir);

This looks like a copy/paste error. Should be c->var_tmp_dir not c->tmp_dir.

> +                c->tmp_dir = NULL;

Ditto.

>  int setup_namespace(
>                  char **writable,
>                  char **readable,
>                  char **inaccessible,
>                  bool private_tmp,
> +                char *tmp_dir,
> +                char *var_tmp_dir,
>                  unsigned long flags) {
>
> -        char
> -                tmp_dir[] = "/tmp/systemd-private-XXXXXX",
> -                var_tmp_dir[] = "/var/tmp/systemd-private-XXXXXX",
> -                inaccessible_dir[] = "/tmp/systemd-inaccessible-XXXXXX";
> +        char inaccessible_dir[] = "/tmp/systemd-inaccessible-XXXXXX";

So /tmp/systemd-inaccessible-* will still not be cleaned up? Why not
just just create one '/tmp/systemd-XXXXXX', which can contain both
'private' and 'inaccessible' as subdirs and clean them up at the same
time?

> +static void service_cleanup_tmpdirs(Service *s) {
> +        assert(s);
> +
> +        exec_context_tmpdirs_done(&s->exec_context);
> +}
> +
>  static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart) {
>          int r;
>          assert(s);
> @@ -2519,6 +2525,8 @@ static int service_stop(Unit *u) {
>                 s->state == SERVICE_EXITED);
>
>          service_enter_stop(s, SERVICE_SUCCESS);
> +        service_cleanup_tmpdirs(s);

Maybe add a comment why this special treatment is necessary for service_stop?

Lastly, I think it would be useful to explain how the lifespan of
PrivateTmp= works in systemd.exec(5).

Cheers,

Tom