[SSSD] [PATCH v2 1/5] NEW CLIENT: plugin for NFSv4 rpc.idmapd

Simo Sorce simo at redhat.com
Mon Jun 23 01:34:54 UTC 2014


On Sun, 2014-06-22 at 23:08 +0200, Jakub Hrozek wrote:
> On 22 Jun 2014, at 22:58, Noam Meltzer <tsnoam at gmail.com> wrote:
> 
> > 
> > 
> > 
> > On Sun, Jun 22, 2014 at 11:28 PM, Jakub Hrozek <jhrozek at redhat.com> wrote:
> > 
> > On 22 Jun 2014, at 22:05, Noam Meltzer <tsnoam at gmail.com> wrote:
> > 
> > > Hi Jakub,
> > >
> > > I rebased my work over b80e8b76, but in order to save some noise on the mailing list, I would like to double check with you a few things. see below my questions.
> > >
> > >  - Noam
> > >
> > >
> > 
> > Sure. Sorry I didn’t reply sooner, but there was little time over the weekend. Today, I also realised one thing I didn’t make clear previously — the SSSD is now in a string freeze mode, so we would be only able to merge the patches after 1.12.0 is released, because the patches modify the man pages. Luckily, 1.12.0 is scheduled for “end of July” and then 1.12.1 comes out in about a month, so we wouldn’t waste too much time. Also, 6 out of 11 tickets, including those with high priority are on review already for the 1.12.0 release, so I believe we are on track.
> > 
> > The code is already split into different patches: actual code, automake & manpages (with the exception for the rpm spec, but I'll ignore it for the sake of my argument). How wrong of me will it be to suggest to commit the code changes now and add the manpage later for 1.12.1?
> > 
> 
> Hm, interesting, normally we commit the whole bunch at the same time, but why not. Please remind us if we forget to include the man page after 1.12.0 is released (sending the man page patch in a separate e-mail would do for a reminder because patchwork would then show a separate thread). 
> 
> We can document the new functionality in release notes (and maybe blog posts etc) so interested users could try it out and it would be properly documented for 1.12.1.
> 
> As a side note, I need to think a bit if the new functionality should only be enabled with —enable-all-experimental-features for 1.12.0 but currently I think it’s not needed. Other opinions are welcome.
> 
> > 
> > Anyhow, thanks for rebasing and thanks for taking the time to reply to my comments!
> > my pleasure :)
> >  
> > 
> > > On Fri, Jun 20, 2014 at 11:53 PM, Noam Meltzer <tsnoam at gmail.com> wrote:
> > >
> > >
> > >
> > > On Fri, Jun 20, 2014 at 5:09 PM, Jakub Hrozek <jhrozek at redhat.com> wrote:
> > > On Tue, Mar 04, 2014 at 09:37:52AM +0200, Noam Meltzer wrote:
> > > > Implementation of design document:
> > > > https://fedorahosted.org/sssd/wiki/DesignDocs/rpc.idmapd%20plugin
> > > > ---
> > > >  src/sss_client/common.c             |   5 +
> > > >  src/sss_client/nfs/sss_nfs_client.c | 569 ++++++++++++++++++++++++++++++++++++
> > > >  src/sss_client/sss_cli.h            |   2 +
> > > >  3 files changed, 576 insertions(+)
> > > >  create mode 100644 src/sss_client/nfs/sss_nfs_client.c
> > > >
> > > > diff --git a/src/sss_client/common.c b/src/sss_client/common.c
> > > > index 6044af0..58a9eca 100644
> > > > --- a/src/sss_client/common.c
> > > > +++ b/src/sss_client/common.c
> > > > @@ -936,6 +936,11 @@ int sss_ssh_make_request(enum sss_cli_command cmd,
> > > >      return ret;
> > > >  }
> > > >
> > > > +int sss_nfs_make_request(enum sss_cli_command cmd, struct sss_cli_req_data *rd,
> > > > +                         uint8_t **rep, size_t *replen, int *errnop)
> > > > +{
> > > > +    return sss_nss_make_request(cmd, rd, rep, replen, errnop);
> > > > +}
> > >
> > > I wonder if we need to add this call at all. Since the pipe we are
> > > talking to is the NSS pipe and we are locking the NSS mutex as well,
> > > maybe it's more readable to drop this call altogether and use
> > > sss_nss_make_request directly?
> > >
> > > I like this "simple" wrapper for a single reason - code
> readability.
> > > at the end, I believe the compiler will "optimise out" this
> function. (though, I haven't actually tested it)

IIRC the calls are both public so the compiler will not optimize either
out I think.

> > Yeah, I’m not worried too much about speed in this case, after all
> there is a ton of malloc()s in the responder code which would be much
> slower than the extra function call. See more below.
> > 
> > > bottom line - let me know what you prefer, and I'll go with you.
> > > Should I remove this wrapper? Or can it be kept?
> > >
> > >
> > 
> > Let me reply with a more precise explanation. What I found strange
> in the code was that the NSS mutex was locked but then there is a call
> to sss_nfs_make_request(). So I would prefer to either also introduce
> functions to lock the “NFS socket” even if it was just an alias for
> the NSS socket for now or get rid of the NFS specific function.

This wouldn't work unless the "nfs locks" are also just aliases of nss
locks, but that would be weird and prone to errors when thinking about
these locks.

>   In other words, I would expect either:
> > 
> > nss_lock()
> > do_stuff_with_nss()
> > nss_unlock()
> > 
> > or the same, just with s/nss/nfs/, but not a mix of the two.
> > 
> > oh.. now i dig you...
> > so I guess adding "sss_nfs_lock()" to common.c with a similar wrapping logic will be what I go for.
> > 
> 
> I would prefer that, yes. 

I would prefer not.

> I am hoping other SSSD developers would chime in as well so that the
> decision is not a one man party, but I personally prefer the code to
> read consistently. If the socket underneath is a shared NSS one or a
> NFS one is an implementation detail.

But it has extremely important consequences on locking and thread
safety, and using aliases, while it may help readability in some areas
can also be quite confusing from the coherency POV.

Simo.


-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list