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

[Tom Gundersen]

On Wed, Feb 13, 2013 at 2:45 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. Same logic applies also to inaccessible directories.

Something I still can't quite understand: you clean /tmp in
service_stop so that it will be clean for when we call service_start.
Is there any reason you don't do this in service_enter_dead instead?
To me that seems much clearer, but I might be missing something (I
tried to chase the state diagram to see if they are equivalent, but I
gave up in the end...).

Also, shouldn't we be cleaning up private tmp for all the other unit
types as well?

A couple of very minor nitpicks inline.

> diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml
> index 53094e5..a33517e 100644
> --- a/man/systemd.exec.xml
> +++ b/man/systemd.exec.xml
> @@ -1107,7 +1107,9 @@
>                                  processes via
>                                  <filename>/tmp</filename> or
>                                  <filename>/var/tmp</filename>
> -                                impossible. Defaults to
> +                                impossible. All temporary data created
> +                                by service will be removed after service

Assuming you agree with the above, this should probably be 'unit'
rather than 'service'? Also, the rest of the paragraph should probably
be rephrased as it talks about the impossibility of sharing /tmp
between processes, but now we can do that, just not with processes not
belonging to the same unit.

> @@ -312,19 +362,10 @@ int setup_namespace(
>          return 0;
>
>  undo_mounts:
> -        for (p = paths; p < paths + n; p++)
> -                if (p->done)
> -                        umount2(p->path, MNT_DETACH);
> +        for (m = context->bind_mounts; m < context->bind_mounts + n; m++)
> +                if (m->done)
> +                        umount2(m->path, MNT_DETACH);
>
>  fail:
> -        if (remove_inaccessible)
> -                rmdir(inaccessible_dir);
> -
> -        if (remove_tmp)
> -                rmdir(tmp_dir);
> -
> -        if (remove_var_tmp)
> -                rmdir(var_tmp_dir);
> -
>          return r;

You could drop the 'fail' label, and just do 'return -errno' instead
of the 'goto'.