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

[Jakub Hrozek]

On 22 Jun 2014, at 22:05, Noam Meltzer <tsnoam(a)gmail.com&gt; 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.

Anyhow, thanks for rebasing and thanks for taking the time to reply to my comments!

>  On Fri, Jun 20, 2014 at 11:53 PM, Noam Meltzer
> <tsnoam(a)gmail.com&gt; wrote:
>  
>  
>  
>  On Fri, Jun 20, 2014 at 5:09 PM, Jakub Hrozek <jhrozek(a)redhat.com&gt; 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) 
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.  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.


>   
>   
>  
>  >
>  >  const char *ssscli_err2string(int err)
>  >  {
>  > diff --git a/src/sss_client/nfs/sss_nfs_client.c
> b/src/sss_client/nfs/sss_nfs_client.c
>  > new file mode 100644
>  > index 0000000..7b23eab
>  > --- /dev/null
>  > +++ b/src/sss_client/nfs/sss_nfs_client.c
>  > @@ -0,0 +1,569 @@
>  > +/*
>  > +   SSSD
>  > +
>  > +   NFS Client
>  > +
>  > +   Copyright (C) Noam Meltzer <noam(a)primarydata.com&gt;    2013-2014
>  
>  This, and other places -- I guess you'd like to update the address to
>  something that exists :-)
>  PrimaryData deserves their credit. I'll add a 2nd line with my personal email :)
>    
OK, fine by me. As long as we can identify the author.

>  
>  > +
>  > +   This program is free software; you can redistribute it and/or modify
>  > +   it under the terms of the GNU General Public License as published by
>  > +   the Free Software Foundation; either version 3 of the License, or
>  > +   (at your option) any later version.
>  > +
>  > +   This program is distributed in the hope that it will be useful,
>  > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>  > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  > +   GNU General Public License for more details.
>  > +
>  > +   You should have received a copy of the GNU General Public License
>  > +   along with this program.  If not, see <http://www.gnu.org/licenses/>;.
>  > +*/
>  > +
>  > +#define _GNU_SOURCE
>  > +
>  > +#include <stddef.h>
>  > +#include <stdlib.h>
>  > +#include <sys/types.h>
>  > +#include <errno.h>
>  > +#include <string.h>
>  > +
>  > +#include <nfsidmap.h>
>  > +#include "nfsidmap_internal.h"
>  > +
>  > +#include "sss_client/sss_cli.h"
>  > +#include "sss_client/nss_mc.h"
>  > +
>  > +
>  > +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
>  > +#define PLUGIN_NAME                 "sss_nfs"
>  > +#define CONF_SECTION                "sss_nfs"
>  > +#define CONF_USE_MC                 "memcache"
>  > +#define REPLY_ID_OFFSET             (8)
>  > +#define REPLY_NAME_OFFSET           (REPLY_ID_OFFSET + 8)
>  > +#define MCBUF_LEN                   (4096)
>  
>  I think this constant is a bit misnamed. The MC prefix suggests that
>  it's only used for memory cache results, which is not true.
>  
>  Will "BUF_LEN" make more sense?
>  
>   
>  I will use BUF_LEN.
>   
Yes,  I agree. I didn’t like the MC prefix, that’s all. Thanks!

>  <snip> 
>  > +static int reply_to_id(id_t *idp, uint8_t *rep, size_t rep_len)
>  > +{
>  > +    int rc = 0;
>  > +    id_t id;
>  > +    uint32_t num_results = 0;
>  > +
>  > +    if (rep_len < sizeof(uint32_t)) {
>  > +        IDMAP_LOG(0, ("%s: reply too small; rep_len=%lu", __func__,
> rep_len));
>  > +        rc = EBADMSG;
>  > +        goto done;
>  > +    }
>  > +
>  > +    SAFEALIGN_COPY_UINT32(&num_results, rep, NULL);
>  > +    if (num_results > 1) {
>  > +        IDMAP_LOG(0, ("%s: too many results (%lu)", __func__,
> num_results));
>  > +        rc = EBADMSG;
>  > +        goto done;
>  > +    }
>  > +    if (num_results == 0) {
>  > +        rc = ENOENT;
>  > +        goto done;
>  > +    }
>  > +    if (rep_len < sizeof(uint32_t) + REPLY_ID_OFFSET) {
>  > +        IDMAP_LOG(0, ("%s: reply too small(2); rep_len=%lu", __func__,
>  > +                      rep_len));
>  > +        rc = EBADMSG;
>  > +        goto done;
>  > +    }
>  
>  Could we use just this ^ check instead of the one at the beginning of
>  the function? It's not like any partial results can be used if the
>  buffer is > sizeof(uint32_t) but < sizeof(uint32_t) + REPLY_ID_OFFSET.
>  
>  the first check ( rep_len < sizeof(uint32_t) ) is mainly defensive programming: what
> if rep_len is 0?
>  if i'm not missing something here, in such a case, SAFEALIGN_COPY_UINT32 will copy
> garbage into num_results, resulting in unexpected behaviour.
>  
>  I'm not sure whether to keep the extra (first) check or to remove it. What do you
> think?
>   
Keeping the checks doesn’t hurt, it was just something that made me stop and re-read the
checks again. Please keep the code in, the compiler might optimise extra checks and if
not, the performance hit is really, really small. Moreover, if we ever split the function
into several, having a check per access might save us from forgetting to check on each
place.

>  <snip>
>