v9fs (9p): syscall setxattr inside kernel 3.14-rc1 returns size of set xattr

v9fs (9p): syscall setxattr inside kernel 3.14-rc1 returns size of set xattr

[DENIEL Philippe]

Hi,

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

     Regards

         Philippe

Re: v9fs (9p): syscall setxattr inside kernel 3.14-rc1 returns size of set xattr

[J. Bruce Fields]

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.

Fwd: v9fs (9p): syscall setxattr inside kernel 3.14-rc1 returns size of set xattr

[Geyslan Gregório Bem]

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