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

[Stefan Priebe - Profihost AG]

Hello Thomas,


Am 09.03.2017 um 18:09 schrieb 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?

*urg* was a leftofter from trying different variations ;-) Sorry.

>> +  cfs_debug("enter cfs_fuse_mode %s", path);
> did you mean:
> enter cfs_fuse_chmod

Yes, fixed for next patch.

>> +  int ret = -ENOSYS;
>> +
> EACCES would be more fitting here?

Sure i was just trying to keep it backward compatible. So currrently you
get not implemented so i kept it like that. Changed to EACCES for V2.

> 
> man 2 chmod
> 
> does not specifies ENOSYS, and implementing it and returning ENOSYS (aka
> not implemented) seems strange to me?

Right but it's the current behaviour of pmxcfs. Changed to EACCES anyway
for V2.

>> +  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

Fixed for V2.

> 
>> +    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
Yes the idea was just to keep the style for each function.

> 
> 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 *
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel