Hi all, Sorry about the late. My inbox was a mess these months. Well, Eric, seems like solved: https://git.kernel.org/cgit/linux/kernel/git/ericvh/v9fs.git/commit/fs/9p/xattr.c?h=for-next&id=f15844e0777fec936f87a87f97394f83911dacd3 Thanks for the patch, Dominique Martinet, and forgive me for the problems indeed. 2014-02-19 11:31 GMT-03:00 Eric Van Hensbergen <ericvh@xxxxxxxxx>: > As nice as returning the offset seems, man page says return 0 on success, so that's probably what we should be following. Actually, even code in v9fs_xattr_set says this. > > The value of returning the offset would be if v9fs_xattr_set was capable of partial write/update of the attribute, but that basically only happens on error if I understand the intent of the code correctly. I suppose there is some value in understanding that a partial write occurred, but the server should be cleaning it up anyways as I would think that xattr operations are supposed to appear atomic even if they are implemented by several underlying protocol operations. > > So we should probably change the code to go back to returning 0 on success. Thanks for delving into this. > > -eric > > > On Wed, Feb 19, 2014 at 8:08 AM, DENIEL Philippe <philippe.deniel@xxxxxx> wrote: >> >> On 02/18/14 19:04, J. Bruce Fields wrote: >> >> On Tue, Feb 18, 2014 at 02:55:59PM +0100, DENIEL Philippe wrote: >> >> I run v9fs as a client on a F20, in front of my Ganesha server (see >> http://github.com/nfs-ganesha for details), using 9p.2000L >> My acl non-regression test showed errors when I installed a recent >> 3.14-rc1 kernel (I got it from kernel.org) on my F20 box. >> Investigation showed that the setfacl command line got messy because >> setxattr() (called from acl_set_modify() in libattr.so) return a >> non-zero value when successful. Further investigation showed that >> this behavior seems to come from v9fs_fid_xattr_set() inside >> fs/9p/xattr.c in the kernel's source. >> >> It seems like setxattr syscall does now return the size of the set >> xattr, and that seems to be the root cause of my problem. I do not >> believe that this change in setxattr is no bug, but a new feature. >> So I guess I should patch my libattr and/or glibc to use xattr/acl >> with kernel 3.14-rc1. >> Question is : where could I get the right version of libattr source >> treee (eventually with libacl if needed). >> >> New kernel features shouldn't break old libraries--sounds like a bug. >> >> --b. >> >> So far, I believe that having setxattr() returning the "offset" of the written xattr is a good behaviour. More or less, we can view setxattr() as a write() (in fact 9p actually "writes" to the xattr). This does not shock me. I just want to have a usable situation. >> >> If I refer to the v9fs git repo (git://git.kernel.org/pub/scm/linux/kernel/git/ericvh/v9fs.git), the new behaviour was introduced by commit bdd5c28dcb8330b9074404cc92a0b83aae5606a9 >> >> commit bdd5c28dcb8330b9074404cc92a0b83aae5606a9 >> Author: Geyslan G. Bem <geyslan@xxxxxxxxx> >> Date: Mon Oct 21 16:47:58 2013 -0300 >> >> 9p: fix return value in case in v9fs_fid_xattr_set() >> >> In case of error in the p9_client_write, the function v9fs_fid_xattr_set >> should return its negative value, what was never being done. >> >> In case of success it only retuned 0. Now it returns the 'offset' >> variable (write_count total). >> >> Signed-off-by: Geyslan G. Bem <geyslan@xxxxxxxxx> >> Signed-off-by: Eric Van Hensbergen <ericvh@xxxxxxxxx> >> >> diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c >> index 3c28cdf..04133a1 100644 >> --- a/fs/9p/xattr.c >> +++ b/fs/9p/xattr.c >> @@ -138,8 +138,7 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, >> if (retval < 0) { >> p9_debug(P9_DEBUG_VFS, "p9_client_xattrcreate failed %d\n", >> retval); >> - p9_client_clunk(fid); >> - return retval; >> + goto err; >> } >> msize = fid->clnt->msize; >> while (value_len) { >> @@ -152,12 +151,15 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, >> if (write_count < 0) { >> /* error in xattr write */ >> retval = write_count; >> - break; >> + goto err; >> } >> offset += write_count; >> value_len -= write_count; >> } >> - return p9_client_clunk(fid); >> + retval = offset; >> +err: >> + p9_client_clunk(fid); >> + return retval; >> } >> >> ssize_t v9fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) >> >> Any comments on this ? >> >> Regards >> >> Philippe >> >> > -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
