[PATCH 1/2] vfs_ceph: use 'file descriptor' version xattr functions when possible

[Ira Cooper]

Two thoughts:

1. I personally don't mind this being case by case, for vfs modules.
2. Regardless of point #1.  If we are going to say "vfs module foo will
obey the 80 col rules." then a cleanup patch should be issued, for it, and
then the policy put into place.

Personally... I want it to be "consistent".  Meaning if a file is "not 80
col" we don't expect authors to write 80 col code.  If a file is.  We
should MAKE it 80 col and enforce it.  Anything in between will confuse
contributors IMHO.

I submit this conversation occurring a second time as an example of that
confusion.  It our job, via the code to be a good example.

Thanks,

-Ira

On Wed, Apr 1, 2015 at 9:22 AM, Michael Adam <obnox at samba.org> wrote:

> On 2015-04-01 at 12:16 +0200, Michael Adam wrote:
> > Hi,
> >
> > Where do the definitions of LIBCEPHFS_VERSION and
> > LIBCEPHFS_VERSION_CODE come from? At least I don't
> > see that in our configure system, and these
> > defines don't show up in our config.h  and I also
> > dont see them in my libcephfs.h.
> > But maybe it is just too old (0.80 on my fedora21).
> > Will these macros appear if the libcephfs.h is new
> > enough?
>
> Ok, answering to myself -- I checked ceph master:
> These macros are there and it seems to be safe to use
> them this way.
>
> The only other point I could make is that it would
> be nice if the added or changed lines would adhere
> to the 80 columns limit if possible.
>
> Not sure if want to be this strict in vfs modules?
>
> Michael
>
> > Apart from that question, the patch looks good to me.
> >
> > Michael
> >
> > On 2015-03-31 at 21:02 +0800, Yan, Zheng wrote:
> > > libcephfs version 0.94 adds 'file descriptor' version xattr functions.
> > > This patch makes corresponding samba VFS callbacks use these new
> > > functions.
> > >
> > > Signed-off-by: Yan, Zheng <zyan at redhat.com>
> > > ---
> > >  source3/modules/vfs_ceph.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/source3/modules/vfs_ceph.c b/source3/modules/vfs_ceph.c
> > > index b3e334e..3b165d1 100644
> > > --- a/source3/modules/vfs_ceph.c
> > > +++ b/source3/modules/vfs_ceph.c
> > > @@ -1071,7 +1071,11 @@ static ssize_t cephwrap_fgetxattr(struct
> vfs_handle_struct *handle, struct files
> > >  {
> > >     int ret;
> > >     DEBUG(10, ("[CEPH] fgetxattr(%p, %p, %s, %p, %llu)\n", handle,
> fsp, name, value, llu(size)));
> > > +#if defined(LIBCEPHFS_VERSION) && LIBCEPHFS_VERSION_CODE >=
> LIBCEPHFS_VERSION(0, 94, 0)
> > > +   ret = ceph_fgetxattr(handle->data, fsp->fh->fd, name, value, size);
> > > +#else
> > >     ret = ceph_getxattr(handle->data, fsp->fsp_name->base_name, name,
> value, size);
> > > +#endif
> > >     DEBUG(10, ("[CEPH] fgetxattr(...) = %d\n", ret));
> > >     if (ret < 0) {
> > >             WRAP_RETURN(ret);
> > > @@ -1112,7 +1116,11 @@ static ssize_t cephwrap_flistxattr(struct
> vfs_handle_struct *handle, struct file
> > >  {
> > >     int ret;
> > >     DEBUG(10, ("[CEPH] flistxattr(%p, %p, %s, %llu)\n", handle, fsp,
> list, llu(size)));
> > > +#if defined(LIBCEPHFS_VERSION) && LIBCEPHFS_VERSION_CODE >=
> LIBCEPHFS_VERSION(0, 94, 0)
> > > +   ret = ceph_flistxattr(handle->data, fsp->fh->fd, list, size);
> > > +#else
> > >     ret = ceph_listxattr(handle->data, fsp->fsp_name->base_name, list,
> size);
> > > +#endif
> > >     DEBUG(10, ("[CEPH] flistxattr(...) = %d\n", ret));
> > >     if (ret < 0) {
> > >             WRAP_RETURN(ret);
> > > @@ -1134,7 +1142,11 @@ static int cephwrap_fremovexattr(struct
> vfs_handle_struct *handle, struct files_
> > >  {
> > >     int ret;
> > >     DEBUG(10, ("[CEPH] fremovexattr(%p, %p, %s)\n", handle, fsp,
> name));
> > > +#if defined(LIBCEPHFS_VERSION) && LIBCEPHFS_VERSION_CODE >=
> LIBCEPHFS_VERSION(0, 94, 0)
> > > +   ret = ceph_fremovexattr(handle->data, fsp->fh->fd, name);
> > > +#else
> > >     ret = ceph_removexattr(handle->data, fsp->fsp_name->base_name,
> name);
> > > +#endif
> > >     DEBUG(10, ("[CEPH] fremovexattr(...) = %d\n", ret));
> > >     WRAP_RETURN(ret);
> > >  }
> > > @@ -1152,7 +1164,11 @@ static int cephwrap_fsetxattr(struct
> vfs_handle_struct *handle, struct files_str
> > >  {
> > >     int ret;
> > >     DEBUG(10, ("[CEPH] fsetxattr(%p, %p, %s, %p, %llu, %d)\n", handle,
> fsp, name, value, llu(size), flags));
> > > +#if defined(LIBCEPHFS_VERSION) && LIBCEPHFS_VERSION_CODE >=
> LIBCEPHFS_VERSION(0, 94, 0)
> > > +   ret = ceph_fsetxattr(handle->data, fsp->fh->fd, name, value, size,
> flags);
> > > +#else
> > >     ret = ceph_setxattr(handle->data, fsp->fsp_name->base_name, name,
> value, size, flags);
> > > +#endif
> > >     DEBUG(10, ("[CEPH] fsetxattr(...) = %d\n", ret));
> > >     WRAP_RETURN(ret);
> > >  }
> > > --
> > > 1.9.3
>
>
>