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

[Michael Adam]

On 2015-04-01 at 09:31 -0400, Ira Cooper wrote:
> 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.

Well, my understanding is that files can be non-80-col or not
adhering to other rules for historic reasons or because reviewers
have not been attentive. This means that since pure reformatting
patches are frowned upon (to put it mildly), the only means of
migrating a source file that is not adhering to 80 col (or whatever
rule) to an adhering state is to enforce the rule for new patches,
i.e. for any added or changed lines.
This way, a file will not be consistent immediately but slowly
moving towards an adhering state -- if it is worked on.
That's the tradeoff.

So I don't think there is a confusion.
But I wanted to ask the question, whether we want to be
as strict for such a vfs module (which can be considered
externally contributed to a major extent) as we are with
our core code.

My personal preference would be: It is in the tree
(and not under third_party/), hence we should be as
strict as with other core code.

Cheers - Michael

> 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
> >
> >
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150401/c388fde4/attachment-0001.pgp>