On Sun, Jun 22, 2014 at 11:28 PM, Jakub Hrozek <jhrozek@redhat.com> wrote:

On 22 Jun 2014, at 22:05, Noam Meltzer <tsnoam@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?


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@gmail.com> wrote:
>
>
>
> On Fri, Jun 20, 2014 at 5:09 PM, Jakub Hrozek <jhrozek@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)

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.

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.

 

>
>
>
> >
> >  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@primarydata.com>    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!
ok.
 

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

 

> <snip>
>