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

[Thomas Lamprecht]

Hi Stefan


On 03/09/2017 08:15 PM, Stefan Priebe - Profihost AG wrote:
> 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.

Good  intention, but here I guess we can ignore that as - hopefully - 
nobody,
relied on the ENOSYS exit code of a chmod/own call in the /etc/pve to do 
some
specified behavior :)

IMO, EACCES says what we want and your patch is an improvement of the 
pmxcfs interface,
as it made no much sense to deny setting the same file permissions it 
currently has (even if programs should not depend on it).


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

Appreciate your effort to keep the style the same.
But for this really short function, where we do not plan to extend it in 
the near future,
I find the way of your v2/v3 nicer.

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