[systemd-devel] [PATCH] tmpfiles: only execute chmod()/chown() when needed

[Lennart Poettering]

On Fri, 11.07.14 15:05, Michael Olbrich (m.olbrich at pengutronix.de) wrote:

> This avoids errors like this, when the paths are already there with the
> correct permissions and owner:
> 
> chmod(/var/spool) failed: Read-only file system
> ---
>  src/tmpfiles/tmpfiles.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
> index 68cfa55..4f41f28 100644
> --- a/src/tmpfiles/tmpfiles.c
> +++ b/src/tmpfiles/tmpfiles.c
> @@ -453,35 +453,41 @@ finish:
>  }
>  
>  static int item_set_perms(Item *i, const char *path) {
> +        struct stat st;
> +        mode_t old_m = ~0;
> +        uid_t old_uid = -1;
> +        gid_t old_gid = -1;

I'd prefer an explicit cast here, to "(uid_t) -1", since uid_t is
actually unsigned, and we should clarify what we are doing here.

> +
>          assert(i);
>          assert(path);
>  
> +        if (stat(path, &st) >= 0) {
> +                old_m = st.st_mode & 07777;
> +                old_uid = st.st_uid;
> +                old_gid = st.st_gid;
> +        }

Hmm, why copy the fields out here? Can't we just initialize them in the
struct directly to something useful, and just always use the structure
directly?

>          /* not using i->path directly because it may be a glob */
>          if (i->mode_set) {
>                  mode_t m = i->mode;
>  
> -                if (i->mask_perms) {
> -                        struct stat st;
> -
> -                        if (stat(path, &st) >= 0) {
> -                                if (!(st.st_mode & 0111))
> -                                        m &= ~0111;
> -                                if (!(st.st_mode & 0222))
> -                                        m &= ~0222;
> -                                if (!(st.st_mode & 0444))
> -                                        m &= ~0444;
> -                                if (!S_ISDIR(st.st_mode))
> -                                        m &= ~07000; /* remove sticky/sgid/suid bit, unless directory */
> -                        }
> +                if (i->mask_perms && old_m != ~0) {

So you are using "old_m != ~0" as check whether the stat() was
successfuly? I'd really prefer if we could make this explicit. Just add
a bool called "st_valid" or so, and check that here. That's certainly
easier to read.

Otherwise looks good!

Lennart

-- 
Lennart Poettering, Red Hat