[pve-devel] [PATCH] implement chown and chmod for user root group www-data and perm 0640

[Thomas Lamprecht]

On 03/09/2017 05:26 PM, Stefan Priebe wrote:
> This allows us to use management software for files inside of /etc/pve.
> f.e. saltstack which rely on being able to set uid,gid and chmod
>
> Signed-off-by: Stefan Priebe <s.priebe at profihost.ag>
> ---
>   data/src/pmxcfs.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/data/src/pmxcfs.c b/data/src/pmxcfs.c
> index 1b6cbcc..aa81808 100644
> --- a/data/src/pmxcfs.c
> +++ b/data/src/pmxcfs.c
> @@ -186,6 +186,43 @@ ret:
>   	return ret;
>   }
>   
> +static int cfs_fuse_chmod(const char *path, mode_t mode)
> +{
> +  const mode_t pve_mode = S_IRUSR | S_IWUSR | S_IRGRP;
> +  int mode_i = mode & (S_IRWXU | S_IRWXG | S_IRWXO);
> +  int pve_mode_i = pve_mode & (S_IRWXU | S_IRWXG | S_IRWXO);
> +
why not directly setting the pve_mode_i variable to

(S_IRUSR | S_IWUSR | S_IRGRP);

?

The expression (S_IRWXU | S_IRWXG | S_IRWXO) equals 0777, so doing a 
binary and to pve_mode does not change anything, or am I mistaken?
> +  cfs_debug("enter cfs_fuse_mode %s", path);
did you mean:
enter cfs_fuse_chmod
?
> +  int ret = -ENOSYS;
> +
EACCES would be more fitting here?

man 2 chmod

does not specifies ENOSYS, and implementing it and returning ENOSYS (aka 
not implemented) seems strange to me?
> +  if (pve_mode_i == mode_i) {
> +    ret = 0;
> +    goto ret;
> +  }
> +
> +  ret:
> +    cfs_debug("leave cfs_fuse_mode %s (%d) mode: %o pve_mode: %o", path, ret, mode_i, pve_mode_i);
> +

it looks like you mix intend styles here (above spaces and below tabs 
for same intend level),
please avoid that

> +	return ret;
> +}

While you adapt the C style we use in other calls, i.e. the ret: label 
and gotos,
it over complicates stuff here as no cleanup must be made

Here a minimal implementation (totally untested™)
--
static int cfs_fuse_chmod(const char *path, mode_t mode)
{
     cfs_debug("enter cfs_fuse_chmod %s", path);
     int ret = -EACCES;

     // asserts 0640, but allows setting UID and GID - some programs 
need that
     if (mode & ACCESSPERMS == (S_IRUSR | S_IWUSR | S_IRGRP))
     {
         ret = 0;
     }
     cfs_debug("leave cfs_fuse_chmod %s (%d) mode: %o pve_mode: %o", 
path, ret, mode_i, pve_mode_i);

     return ret;
}
--


> +
> +static int cfs_fuse_chown(const char *path, uid_t user, gid_t group)
> +{
> +	cfs_debug("enter cfs_fuse_chown %s", path);
> +
> +	int ret = -ENOSYS;
Also EACCES here?

same here (above tabs and below spaces for same intend level),

rest looks ok.
> +
> +    if (user == 0 && group == cfs.gid) {
> +      ret = 0;
> +      goto ret;
> +    }
> +
> +    ret:
> +      cfs_debug("leave cfs_fuse_chown %s (%d)", path, ret);
> +
> +    return ret;
> +}
> +
>   static int cfs_fuse_mkdir(const char *path, mode_t mode)
>   {
>   	cfs_debug("enter cfs_fuse_mkdir %s", path);
> @@ -488,7 +525,9 @@ static struct fuse_operations fuse_ops = {
>   	.readlink = cfs_fuse_readlink,
>   	.utimens = cfs_fuse_utimens,
>   	.statfs = cfs_fuse_statfs,
> -	.init = cfs_fuse_init
> +	.init = cfs_fuse_init,
> +  .chown = cfs_fuse_chown,
> +  .chmod = cfs_fuse_chmod
>   };
>   
>   static char *