Hi Thomas, thanks and yes if you will do a V5 it would be great!
Stefan Am 21.03.2017 um 10:46 schrieb Thomas Lamprecht: > Hi, > > On 03/20/2017 03:11 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 >> >> Reviewed-by: Thomas Lamprecht <t.lampre...@proxmox.com> > > If you change the patch behavior (comments or other non semantic changes > are normally fine) without previous discussion with the old reviewed-by > people > I'd suggest dropping them when sending it, as else it suggest that the > new behavior > and code was also already reviewed. I know I'm nitpicking for such a > "small" change, > but the cluster filesystem is a core part of ours and permission should > work. > Anyways, would be great for the next time :) > >> Signed-off-by: Stefan Priebe <s.pri...@profihost.ag> >> --- >> data/src/pmxcfs.c | 38 +++++++++++++++++++++++++++++++++++++- >> 1 file changed, 37 insertions(+), 1 deletion(-) >> >> diff --git a/data/src/pmxcfs.c b/data/src/pmxcfs.c >> index 1b6cbcc..5f45115 100644 >> --- a/data/src/pmxcfs.c >> +++ b/data/src/pmxcfs.c >> @@ -186,6 +186,40 @@ ret: >> return ret; >> } >> +static int cfs_fuse_chmod(const char *path, mode_t mode) >> +{ >> + int ret = -EACCES; >> + >> + cfs_debug("enter cfs_fuse_chmod %s", path); >> + >> + // asserts 0640 or 0600, but allows setting UID and GID - some >> programs need that >> + if (path_is_private(path)) { >> + if ((mode & ACCESSPERMS) == (S_IRUSR | S_IWUSR)) >> + ret = 0; >> + } else if ((mode & ACCESSPERMS) == (S_IRUSR | S_IWUSR | S_IRGRP)) { >> + ret = 0; >> + } >> + > > With the new addition, may I propose another refactoring of above, > as IMO nested if branches with no parenthesis in the inner level seems > confusing. > (I generally do not like the "no-parenthesis if one liners", but it > seems they are > widely used here, so I also do not want to break with the style.) > > Whats your opinion on something like: > > -- > static int cfs_fuse_chmod(const char *path, mode_t mode) > { > int ret = -EACCES; > > cfs_debug("enter cfs_fuse_chmod %s", path); > > mode_t allowed_mode = (S_IRUSR | S_IWUSR); > if (!path_is_private(path)) > allowed_mode |= (S_IRGRP); > > // ACCESSPERMS masks out UID and GID for the check, some > programs need to set them > if ((mode & ACCESSPERMS) == allowed_mode) > ret = 0; > > cfs_debug("leave cfs_fuse_chmod %s (%d) mode: %o", path, ret, > (int)mode); > > return ret; > } > -- > > Else, all looks good. I can also make a V5 with those changes on top of > yours, if you prefer. > > cheers, > Thomas > >> + cfs_debug("leave cfs_fuse_chmod %s (%d) mode: %o", path, ret, >> (int)mode); >> + >> + return ret; >> +} >> + >> +static int cfs_fuse_chown(const char *path, uid_t user, gid_t group) >> +{ >> + int ret = -EACCES; >> + >> + cfs_debug("enter cfs_fuse_chown %s", path); >> + >> + // we get -1 if no change should be made >> + if ((user == 0 || user == -1) && (group == cfs.gid || group == -1)) >> + ret = 0; >> + >> + cfs_debug("leave cfs_fuse_chown %s (%d) (uid: %d; gid: %d)", >> path, ret, user, group); >> + >> + return ret; >> +} >> + >> static int cfs_fuse_mkdir(const char *path, mode_t mode) >> { >> cfs_debug("enter cfs_fuse_mkdir %s", path); >> @@ -488,7 +522,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@pve.proxmox.com > http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel