<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jun 20, 2014 at 5:09 PM, Jakub Hrozek <span dir="ltr">&lt;<a href="mailto:jhrozek@redhat.com" target="_blank">jhrozek@redhat.com</a>&gt;</span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On Tue, Mar 04, 2014 at 09:37:52AM +0200, Noam Meltzer wrote:<br>
&gt; Implementation of design document:<br>
&gt; <a href="https://fedorahosted.org/sssd/wiki/DesignDocs/rpc.idmapd%20plugin" target="_blank">https://fedorahosted.org/sssd/wiki/DesignDocs/rpc.idmapd%20plugin</a><br>
&gt; ---<br>
&gt;  src/sss_client/common.c             |   5 +<br>
&gt;  src/sss_client/nfs/sss_nfs_client.c | 569 ++++++++++++++++++++++++++++++++++++<br>
&gt;  src/sss_client/sss_cli.h            |   2 +<br>
&gt;  3 files changed, 576 insertions(+)<br>
&gt;  create mode 100644 src/sss_client/nfs/sss_nfs_client.c<br>
&gt;<br>
&gt; diff --git a/src/sss_client/common.c b/src/sss_client/common.c<br>
&gt; index 6044af0..58a9eca 100644<br>
&gt; --- a/src/sss_client/common.c<br>
&gt; +++ b/src/sss_client/common.c<br>
&gt; @@ -936,6 +936,11 @@ int sss_ssh_make_request(enum sss_cli_command cmd,<br>
&gt;      return ret;<br>
&gt;  }<br>
&gt;<br>
&gt; +int sss_nfs_make_request(enum sss_cli_command cmd, struct sss_cli_req_data *rd,<br>
&gt; +                         uint8_t **rep, size_t *replen, int *errnop)<br>
&gt; +{<br>
&gt; +    return sss_nss_make_request(cmd, rd, rep, replen, errnop);<br>
&gt; +}<br>
<br>
</div>I wonder if we need to add this call at all. Since the pipe we are<br>
talking to is the NSS pipe and we are locking the NSS mutex as well,<br>
maybe it&#39;s more readable to drop this call altogether and use<br>
sss_nss_make_request directly?<br></blockquote><div><br></div><div>I like this &quot;simple&quot; wrapper for a single reason - code readability.</div><div>at the end, I believe the compiler will &quot;optimise out&quot; this function. (though, I haven&#39;t actually tested it)</div>
<div>bottom line - let me know what you prefer, and I&#39;ll go with you.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
&gt;<br>
&gt;  const char *ssscli_err2string(int err)<br>
&gt;  {<br>
&gt; diff --git a/src/sss_client/nfs/sss_nfs_client.c b/src/sss_client/nfs/sss_nfs_client.c<br>
&gt; new file mode 100644<br>
&gt; index 0000000..7b23eab<br>
&gt; --- /dev/null<br>
&gt; +++ b/src/sss_client/nfs/sss_nfs_client.c<br>
&gt; @@ -0,0 +1,569 @@<br>
&gt; +/*<br>
&gt; +   SSSD<br>
&gt; +<br>
&gt; +   NFS Client<br>
&gt; +<br>
&gt; +   Copyright (C) Noam Meltzer &lt;<a href="mailto:noam@primarydata.com">noam@primarydata.com</a>&gt;    2013-2014<br>
<br>
</div>This, and other places -- I guess you&#39;d like to update the address to<br>
something that exists :-)<br></blockquote><div>PrimaryData deserves their credit. I&#39;ll add a 2nd line with my personal email :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div><div class="h5"><br>
&gt; +<br>
&gt; +   This program is free software; you can redistribute it and/or modify<br>
&gt; +   it under the terms of the GNU General Public License as published by<br>
&gt; +   the Free Software Foundation; either version 3 of the License, or<br>
&gt; +   (at your option) any later version.<br>
&gt; +<br>
&gt; +   This program is distributed in the hope that it will be useful,<br>
&gt; +   but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
&gt; +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the<br>
&gt; +   GNU General Public License for more details.<br>
&gt; +<br>
&gt; +   You should have received a copy of the GNU General Public License<br>
&gt; +   along with this program.  If not, see &lt;<a href="http://www.gnu.org/licenses/" target="_blank">http://www.gnu.org/licenses/</a>&gt;.<br>
&gt; +*/<br>
&gt; +<br>
&gt; +#define _GNU_SOURCE<br>
&gt; +<br>
&gt; +#include &lt;stddef.h&gt;<br>
&gt; +#include &lt;stdlib.h&gt;<br>
&gt; +#include &lt;sys/types.h&gt;<br>
&gt; +#include &lt;errno.h&gt;<br>
&gt; +#include &lt;string.h&gt;<br>
&gt; +<br>
&gt; +#include &lt;nfsidmap.h&gt;<br>
&gt; +#include &quot;nfsidmap_internal.h&quot;<br>
&gt; +<br>
&gt; +#include &quot;sss_client/sss_cli.h&quot;<br>
&gt; +#include &quot;sss_client/nss_mc.h&quot;<br>
&gt; +<br>
&gt; +<br>
&gt; +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
&gt; +#define PLUGIN_NAME                 &quot;sss_nfs&quot;<br>
&gt; +#define CONF_SECTION                &quot;sss_nfs&quot;<br>
&gt; +#define CONF_USE_MC                 &quot;memcache&quot;<br>
&gt; +#define REPLY_ID_OFFSET             (8)<br>
&gt; +#define REPLY_NAME_OFFSET           (REPLY_ID_OFFSET + 8)<br>
&gt; +#define MCBUF_LEN                   (4096)<br>
<br>
</div></div>I think this constant is a bit misnamed. The MC prefix suggests that<br>
it&#39;s only used for memory cache results, which is not true.<br></blockquote><div><br></div><div>Will &quot;BUF_LEN&quot; make more sense?</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div><div class="h5"><br>
&gt; +#define USE_MC_DEFAULT              true<br>
&gt; +<br>
&gt; +<br>
&gt; +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
&gt; +static char sss_nfs_plugin_name[]   = PLUGIN_NAME;<br>
&gt; +static char nfs_conf_sect[]         = CONF_SECTION;<br>
&gt; +static char nfs_conf_use_mc[]       = CONF_USE_MC;<br>
&gt; +<br>
&gt; +static bool nfs_use_mc              = USE_MC_DEFAULT;<br>
&gt; +<br>
&gt; +<br>
&gt; +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
&gt; +/* Forward declarations */<br>
&gt; +static int send_recv(uint8_t **repp, size_t *rep_lenp, enum sss_cli_command cmd,<br>
&gt; +                     const void *req, size_t req_len);<br>
&gt; +static int reply_to_id(id_t *idp, uint8_t *rep, size_t rep_len);<br>
&gt; +static int reply_to_name(char *name, size_t len, uint8_t *rep, size_t rep_len);<br>
&gt; +<br>
&gt; +<br>
&gt; +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
&gt; +/* get from memcache functions */<br>
&gt; +static int get_uid_from_mc(id_t *uid, const char *name)<br>
&gt; +{<br>
&gt; +    int rc = 0;<br>
&gt; +    struct passwd pwd;<br>
&gt; +    char *buf = NULL;<br>
&gt; +    char *p = NULL;<br>
&gt; +    size_t buflen = 0;<br>
&gt; +    size_t len = 0;<br>
&gt; +<br>
&gt; +    if (!nfs_use_mc) {<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    sss_strnlen(name, SSS_NAME_MAX, &amp;len);<br>
&gt; +<br>
&gt; +    do {<br>
&gt; +        buflen += MCBUF_LEN;<br>
&gt; +        if ((p = realloc(buf, buflen)) == NULL) {<br>
&gt; +            rc = ENOMEM;<br>
&gt; +            goto done;<br>
&gt; +        }<br>
&gt; +        buf = p;<br>
&gt; +        rc = sss_nss_mc_getpwnam(name, len, &amp;pwd, buf, buflen);<br>
&gt; +    } while (rc == ERANGE);<br>
&gt; +<br>
&gt; +    if (rc == 0) {<br>
&gt; +        IDMAP_LOG(1, (&quot;found user %s in memcache&quot;, name));<br>
&gt; +        *uid = pwd.pw_uid;<br>
&gt; +    } else {<br>
&gt; +        IDMAP_LOG(1, (&quot;user %s not in memcache&quot;, name));<br>
&gt; +    }<br>
&gt; +<br>
&gt; +done:<br>
&gt; +    free(buf);<br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int get_gid_from_mc(id_t *gid, const char *name) {<br>
</div></div>                                                      ^^^^^^^^<br>
We prefer to put the opening &quot;{&quot; after a newline for function<br>
declarations.<br></blockquote><div><br></div><div>so do I. my bad :)</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
&gt; +    int rc = 0;<br>
&gt; +    struct group grp;<br>
&gt; +    char *buf = NULL;<br>
&gt; +    char *p = NULL;<br>
&gt; +    size_t buflen = 0;<br>
&gt; +    size_t len;<br>
&gt; +<br>
&gt; +    if (!nfs_use_mc) {<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    sss_strnlen(name, SSS_NAME_MAX, &amp;len);<br>
&gt; +<br>
&gt; +    do {<br>
&gt; +        buflen += MCBUF_LEN;<br>
&gt; +        if ((p = realloc(buf, buflen)) == NULL) {<br>
&gt; +            rc = ENOMEM;<br>
&gt; +            goto done;<br>
&gt; +        }<br>
&gt; +        buf = p;<br>
&gt; +        rc = sss_nss_mc_getgrnam(name, len, &amp;grp, buf, buflen);<br>
&gt; +    } while (rc == ERANGE);<br>
&gt; +<br>
&gt; +    if (rc == 0) {<br>
&gt; +        IDMAP_LOG(1, (&quot;found group %s in memcache&quot;, name));<br>
&gt; +        *gid = grp.gr_gid;<br>
&gt; +    } else {<br>
&gt; +        IDMAP_LOG(1, (&quot;group %s not in memcache&quot;, name));<br>
&gt; +    }<br>
&gt; +<br>
&gt; +done:<br>
&gt; +    free(buf);<br>
&gt; +    return rc;<br>
&gt; +}<br>
<br>
</div></div>[snip]<br>
<div><div class="h5"><br>
&gt; +static int reply_to_id(id_t *idp, uint8_t *rep, size_t rep_len)<br>
&gt; +{<br>
&gt; +    int rc = 0;<br>
&gt; +    id_t id;<br>
&gt; +    uint32_t num_results = 0;<br>
&gt; +<br>
&gt; +    if (rep_len &lt; sizeof(uint32_t)) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: reply too small; rep_len=%lu&quot;, __func__, rep_len));<br>
&gt; +        rc = EBADMSG;<br>
&gt; +        goto done;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    SAFEALIGN_COPY_UINT32(&amp;num_results, rep, NULL);<br>
&gt; +    if (num_results &gt; 1) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: too many results (%lu)&quot;, __func__, num_results));<br>
&gt; +        rc = EBADMSG;<br>
&gt; +        goto done;<br>
&gt; +    }<br>
&gt; +    if (num_results == 0) {<br>
&gt; +        rc = ENOENT;<br>
&gt; +        goto done;<br>
&gt; +    }<br>
&gt; +    if (rep_len &lt; sizeof(uint32_t) + REPLY_ID_OFFSET) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: reply too small(2); rep_len=%lu&quot;, __func__,<br>
&gt; +                      rep_len));<br>
&gt; +        rc = EBADMSG;<br>
&gt; +        goto done;<br>
&gt; +    }<br>
<br>
</div></div>Could we use just this ^ check instead of the one at the beginning of<br>
the function? It&#39;s not like any partial results can be used if the<br>
buffer is &gt; sizeof(uint32_t) but &lt; sizeof(uint32_t) + REPLY_ID_OFFSET.<br>
<div><div class="h5"><br></div></div></blockquote><div>the first check ( rep_len &lt; sizeof(uint32_t) ) is mainly defensive programming: what if rep_len is 0?</div><div>if i&#39;m not missing something here, in such a case, SAFEALIGN_COPY_UINT32 will copy garbage into num_results, resulting in unexpected behaviour.</div>
<div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
&gt; +<br>
&gt; +    SAFEALIGN_COPY_UINT32(&amp;id, rep + REPLY_ID_OFFSET, NULL);<br>
&gt; +    *idp = id;<br>
&gt; +<br>
&gt; +done:<br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int reply_to_name(char *name, size_t len, uint8_t *rep, size_t rep_len)<br>
&gt; +{<br>
&gt; +    int rc = 0;<br>
&gt; +    uint32_t num_results = 0;<br>
&gt; +    const char *buf;<br>
&gt; +    size_t buf_len;<br>
&gt; +    size_t offset;<br>
&gt; +<br>
&gt; +    if (rep_len &lt; sizeof(uint32_t)) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: reply too small; rep_len=%lu&quot;, __func__, rep_len));<br>
&gt; +        rc = EBADMSG;<br>
&gt; +        goto done;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    SAFEALIGN_COPY_UINT32(&amp;num_results, rep, NULL);<br>
&gt; +    if (num_results &gt; 1) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: too many results (%lu)&quot;, __func__, num_results));<br>
&gt; +        rc = EBADMSG;<br>
&gt; +        goto done;<br>
&gt; +    }<br>
&gt; +    if (num_results == 0) {<br>
&gt; +        rc = ENOENT;<br>
&gt; +        goto done;<br>
&gt; +    }<br>
&gt; +    if (rep_len &lt; sizeof(uint32_t) + REPLY_ID_OFFSET) {<br>
<br>
</div></div>I think this is a typo, shouldn&#39;t the length be checked against<br>
REPLY_NAME_OFFSET instead?<br></blockquote><div>indeed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
&gt; +        IDMAP_LOG(0, (&quot;%s: reply too small(2); rep_len=%lu&quot;, __func__,<br>
&gt; +                      rep_len));<br>
&gt; +        rc = EBADMSG;<br>
&gt; +        goto done;<br>
&gt; +    }<br>
<br>
</div>Similar comment about the length check.<br></blockquote><div>similar answer :)</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
&gt; +<br>
&gt; +    buf = (const char *)(rep + REPLY_NAME_OFFSET);<br>
&gt; +    buf_len = rep_len - REPLY_NAME_OFFSET;<br>
&gt; +    offset = 0;<br>
&gt; +    rc = sss_readrep_copy_string(buf, &amp;offset, &amp;buf_len, &amp;len, &amp;name, NULL);<br>
&gt; +    if (rc != 0) {<br>
&gt; +        rc = -rc;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +done:<br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
&gt; +/* configuration parsing aids */<br>
&gt; +static bool str_equal(const char *s1, const char *s2)<br>
&gt; +{<br>
&gt; +    bool res = false;<br>
&gt; +    size_t len1;<br>
&gt; +    size_t len2;<br>
&gt; +<br>
&gt; +    len1 = strlen(s1);<br>
&gt; +    len2 = strlen(s2);<br>
&gt; +<br>
&gt; +    if (len1 == len2) {<br>
&gt; +        res = (strncasecmp(s1, s2, len1) == 0);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return res;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int nfs_conf_get_bool(char *sect, char *attr, int def)<br>
&gt; +{<br>
&gt; +    int res;<br>
&gt; +    char *val;<br>
&gt; +<br>
&gt; +    res = def;<br>
&gt; +    val = conf_get_str(sect, attr);<br>
&gt; +    if (val) {<br>
&gt; +        res = (str_equal(&quot;1&quot;, val) ||<br>
&gt; +               str_equal(&quot;yes&quot;, val) ||<br>
&gt; +               str_equal(&quot;true&quot;, val) ||<br>
&gt; +               str_equal(&quot;on&quot;, val));<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return res;<br>
&gt; +}<br>
&gt; +<br>
&gt; +<br>
&gt; +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
&gt; +/* libnfsidmap return-code aids */<br>
&gt; +<br>
&gt; +/*<br>
&gt; + * we only want to return 0 or ENOENT; otherwise libnfsidmap will stop<br>
&gt; + * translation instead of proceeding to the next translation plugin<br>
&gt; + */<br>
&gt; +int normalise_rc(int rc) {<br>
</div></div>                        ^^^^^^^<br>
                    Same comment like get_gid_from_mc<br>
<div class="">&gt; +    int res;<br>
&gt; +<br>
&gt; +    res = rc;<br>
&gt; +    if (res != 0 &amp;&amp; res != ENOENT) {<br>
&gt; +        res = ENOENT;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return res;<br>
&gt; +}<br>
&gt; +<br>
&gt; +/* log the actual rc from our code (to be used before normalising the rc) */<br>
&gt; +void log_actual_rc(const char *trans_name, int rc) {<br>
</div>                                                ^^^^^^^^^<br>
                                        And here.<br>
<div class="">&gt; +    char tmp[80];<br>
&gt; +    IDMAP_LOG(1, (&quot;%s: rc=%i msg=%s&quot;, trans_name, rc,<br>
&gt; +                  strerror_r(rc, tmp, sizeof(tmp))));<br>
&gt; +}<br>
&gt; +<br>
&gt; +<br>
<br>
</div>The rest of the file looks good to me.<br>
</blockquote></div><br></div></div>