* Fwd: v9fs (9p): syscall setxattr inside kernel 3.14-rc1 returns size of set xattr
[not found] ` <CAGG-pUSEsThPiE+3sm75GVoE7QwuEhwRnT+0+2=osFdKLbQMOA@mail.gmail.com>
@ 2014-08-21 11:10 ` Geyslan Gregório Bem
0 siblings, 0 replies; 3+ messages in thread
From: Geyslan Gregório Bem @ 2014-08-21 11:10 UTC (permalink / raw)
To: open list:FILESYSTEMS (VFS...), linux-kernel@vger.kernel.org (open list)
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@gmail.com>:
> 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@cea.fr> 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@gmail.com>
>> 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@gmail.com>
>> Signed-off-by: Eric Van Hensbergen <ericvh@gmail.com>
>>
>> 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
^ permalink raw reply [flat|nested] 3+ messages in thread