<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Jan 26, 2014 at 1:10 AM, Simo Sorce <span dir="ltr">&lt;<a href="mailto:simo@redhat.com" target="_blank">simo@redhat.com</a>&gt;</span> wrote:<br>



<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I have to Nack this one.<br>
<br>
I will check the following patches in the next few days, but I am<br>
traveling so it may take a while.<br>
<br>
Comments inline.<br></blockquote><div><br></div><div>Hi,<br></div><div>Sorry for the delay with my answer.<br><br></div><div>Comments inline.<br></div><div>For code style/conventions I haven&#39;t added comments. I&#39;ll just go with your conventions.<br>
<br></div><div>- Noam<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div><br>
On Thu, 2014-01-16 at 11:02 +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 | 490 ++++++++++++++++++++++++++++++++++++<br>
&gt;  src/sss_client/sss_cli.h            |   2 +<br>
&gt;  3 files changed, 497 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>
&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..9f6e047<br>
&gt; --- /dev/null<br>
&gt; +++ b/src/sss_client/nfs/sss_nfs_client.c<br>
&gt; @@ -0,0 +1,490 @@<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" target="_blank">noam@primarydata.com</a>&gt;    2013<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; +#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; +#include &quot;cfg.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 SSS_NFS_PLUGIN_NAME         &quot;sss_nfs&quot;<br>
&gt; +#define SSS_NFS_CONF_SECTION        &quot;sss_nfs&quot;<br>
&gt; +#define SSS_NFS_CONF_USE_MC         &quot;memcache&quot;<br>
&gt; +#define SSS_NFS_REPLY_ID_OFFSET     (8)<br>
&gt; +#define SSS_NFS_REPLY_NAME_OFFSET   (SSS_NFS_REPLY_ID_OFFSET + 8)<br>
&gt; +#define SSS_NFS_MCBUF_LEN           (4096)<br>
&gt; +#define SSS_NFS_USE_MC_DEFAULT      (1)<br>
&gt; +<br>
&gt; +<br>
&gt; +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
&gt; +static char s_sss_nfs_plugin_name[]     = SSS_NFS_PLUGIN_NAME;<br>
&gt; +static char s_sss_nfs_conf_section[]    = SSS_NFS_CONF_SECTION;<br>
&gt; +static char s_sss_nfs_conf_use_mc[]     = SSS_NFS_CONF_USE_MC;<br>
<br>
</div></div>What does the s_ stand for ?<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">



We use sss_ as a namespacing measure, but it is strictly necessary only<br>
for non static functions or variables.<br>
We do not use s_sss_ for anything though, and we do not use hungarian<br>
notations or similar.<br>
<br>
Anyway it is unclear to me what is the point of defining a static string<br>
that contains a const string ?<br></blockquote><div><br>The libnfsidmap API is using &quot;char *&quot; (without const) where ever there&#39;s a need to pass a string. (for example, check the prototype of &quot;conf_get_str()&quot;)<br>

</div><div>Just using a macro in these places will cause the following warning to be emitted: &quot;initialization discards &#39;const&#39; qualifier from pointer target type&quot;.<br></div><div>So I find that the cleanest approach to handle this would be to define a &quot;static char[]&quot; for each of these instances.<br>

<br></div><div>Now, personally I prefer to do something like this:<br></div><div>#define PLUGIN_NAME &quot;sss_nfs&quot;<br></div><div>static char sss_nfs_plugin_name[] = PLUGIN_NAME;<br><br></div><div>rather than:<br></div>

<div>static char sss_nfs_plugin_name[] = &quot;sss_nfs&quot;;<br><br></div><div>but I don&#39;t mind taking the second approach if that&#39;s more aligned with sssd&#39;s style/conventions.<br></div><div>

</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +static char *s_mcbuf    = NULL;<br>
<br>
</div>You are defining s_mcbuf as a global, then using it liberally in<br>
multiple calls.<br>
<br>
This is not reentrant nor thread safe, the client code MUST be thread<br>
safe.<br></blockquote><div><br></div><div>rpc.idmapd is not multi-threaded that&#39;s why I allowed myself this liberty.<br></div><div>however, reading on I see that using dynamic allocation is a must, so this is an actual bug.<br>
<br></div><div>Anyway, if the client have to be reentrant, I&#39;ll also fix the locations using &quot;strerror&quot; to &quot;strerror_r&quot;.<br>


</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +static int   s_use_mc   = SSS_NFS_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 get_id_from_rep(id_t *idp, uint8_t *rep, size_t rep_len);<br>
<br>
</div>Please rename to get_id_from_reply() or reply_to_id()<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +static int get_name_from_rep(char *name, size_t len, uint8_t *rep,<br>
&gt; +                             size_t rep_len);<br>
<br>
</div>Please rename to get_name_from_reply() or reply_to_name()<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><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;<br>
&gt; +    struct passwd pwd;<br>
&gt; +    char *buf;<br>
<br>
</div>Use char buf[SSS_NFS_MCBUF_LEN]; here instead of a globally allocate<br>
buffer.<br>
<br>
It&#39;s unlikely a passwd structure will be bigger than 4K but to be<br>
absolutely accurate you may want to do what I suggest later for the<br>
group entries (for those it is mandatory).<br></blockquote><div>ok<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +    size_t len = 0;<br>
&gt; +<br>
&gt; +    if (!s_use_mc) {<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    buf = s_mcbuf;<br>
&gt; +    sss_strnlen(name, SSS_NAME_MAX, &amp;len);<br>
<br>
</div>Missing error checking here, see _nss_sss_getpwnam_r()<br></blockquote><div><div><br>not quite. the actual validation is done in the &quot;entry&quot; functions (sss_nfs_name_to_[ug]id(), sss_nfs_[ug]id_to_name()).<br>
This and the other 3 locations mentioned below are internal functions, so it  should
 be safe to assume a sanitised input.<br>


</div><div>The use of &quot;sss_strnlen()&quot; here is only needed in order to know the length of the string.<br><br></div>Another
 option is to pass the (already calculated) length as an argument to the
 function. However, I find it somewhat cumbersome, so I prefer the extra 
strnlen.<br><br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">



<div><br>
&gt; +    rc = sss_nss_mc_getpwnam(name, len, &amp;pwd, buf, SSS_NFS_MCBUF_LEN);<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; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int get_gid_from_mc(id_t *gid, const char *name) {<br>
&gt; +    int rc;<br>
&gt; +    struct group grp;<br>
&gt; +    char *buf;<br>
&gt; +    size_t len;<br>
&gt; +<br>
&gt; +    if (!s_use_mc) {<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    buf = s_mcbuf;<br>
&gt; +    sss_strnlen(name, SSS_NAME_MAX, &amp;len);<br>
<br>
</div>No error checking as above here ^<br>
<br>
For the following call using a fixed size buffer will not do, because<br>
group memberships may be quite large in some environments.<br>
<br>
You can use it for a first call, but if ERANGE is returned you must<br>
retry with a bigger, allocated buffer.<br>
<br>
You are not doing the retry at all now, which is a bug. <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +    rc = sss_nss_mc_getgrnam(name, len, &amp;grp, buf, SSS_NFS_MCBUF_LEN);<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; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int get_user_from_mc(char *name, size_t len, uid_t uid)<br>
&gt; +{<br>
&gt; +    int rc;<br>
&gt; +    struct passwd pwd;<br>
&gt; +    char *buf;<br>
&gt; +    size_t cache_len;<br>
<br>
</div>Pleas rename to pw_name_len<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +    if (!s_use_mc) {<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    buf = s_mcbuf;<br>
<br>
</div>Same comments as per get_uid_from_mc()<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +    rc = sss_nss_mc_getpwuid(uid, &amp;pwd, buf, SSS_NFS_MCBUF_LEN);<br>
&gt; +    if (rc == 0) {<br>
&gt; +        cache_len = strlen(pwd.pw_name) + 1;<br>
&gt; +        if (cache_len &gt; len) {<br>
&gt; +            IDMAP_LOG(0, (&quot;%s: reply too long; cache_len=%lu, len=%lu&quot;,<br>
&gt; +                          __func__, cache_len, len));<br>
&gt; +            rc = -ENOBUFS;<br>
&gt; +        }<br>
&gt; +        memcpy(name, pwd.pw_name, cache_len);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int get_group_from_mc(char *name, size_t len, id_t gid)<br>
&gt; +{<br>
&gt; +    int rc;<br>
&gt; +    struct group grp;<br>
&gt; +    char *buf;<br>
&gt; +    size_t cache_len;<br>
&gt; +<br>
&gt; +    buf = s_mcbuf;<br>
<br>
</div>Same comments as per get_gid_from_mc()<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +    rc = sss_nss_mc_getgrgid(gid, &amp;grp, buf, SSS_NFS_MCBUF_LEN);<br>
&gt; +    if (rc == 0) {<br>
&gt; +        cache_len = strlen(grp.gr_name) + 1;<br>
&gt; +        if (cache_len &gt; len) {<br>
&gt; +            IDMAP_LOG(0, (&quot;%s: reply too long; cache_len=%lu, len=%lu&quot;,<br>
&gt; +                          __func__, cache_len, len));<br>
&gt; +            rc = -ENOBUFS;<br>
&gt; +        }<br>
&gt; +        memcpy(name, grp.gr_name, cache_len);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
&gt; +static int name_to_id(const char *name, id_t *id, enum sss_cli_command cmd)<br>
&gt; +{<br>
&gt; +    int rc;<br>
&gt; +    uint8_t *rep = NULL;<br>
&gt; +    size_t rep_len = 0;<br>
&gt; +    size_t name_len;<br>
&gt; +<br>
&gt; +    sss_strnlen(name, SSS_NAME_MAX, &amp;name_len);<br>
<br>
</div>missing error checking</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +    rc = send_recv(&amp;rep, &amp;rep_len, cmd, name, name_len + 1);<br>
&gt; +    if (rc == 0) {<br>
&gt; +        rc = get_id_from_rep(id, rep, rep_len);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    if (rep != NULL) {<br>
<br>
</div>Don&#39;t test for rep, just call free() straight, glibc knows to ignore<br>
NULLs.<br></blockquote><div><br></div><div>ok.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +        free(rep);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int id_to_name(char *name, size_t len, id_t id,<br>
&gt; +                      enum sss_cli_command cmd)<br>
&gt; +{<br>
&gt; +    int rc;<br>
&gt; +    const void *req = NULL;<br>
&gt; +    uint8_t *rep = NULL;<br>
&gt; +    size_t req_len;<br>
&gt; +    size_t rep_len = 0;<br>
&gt; +<br>
&gt; +    req = (void *)(&amp;id);<br>
&gt; +    req_len = sizeof(id);<br>
<br>
</div>This is not safe on some architectures (like ARM).<br>
<br>
do this instead:<br>
<br>
size_t req_len = sizeof(id_t);<br>
uint8_t req[req_len];<br>
<br>
memcpy(req, &amp;id, req_len)<br></blockquote><div><br></div><div>Good to know. Is there any place where I can read about this difference?<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

<div><br>
&gt; +    rc = send_recv(&amp;rep, &amp;rep_len, cmd, req, req_len);<br>
&gt; +    if (rc == 0) {<br>
&gt; +        rc = get_name_from_rep(name, len, rep, rep_len);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    if (rep != NULL) {<br>
&gt; +        free(rep);<br>
&gt; +    }<br>
<br>
</div>Just call free() w/o checks.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +<br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
&gt; +static int send_recv(uint8_t **repp, size_t *rep_lenp, enum sss_cli_command cmd,<br>
<br>
<br>
</div>Why send_recv instead of just calling it sss_nfs_make_request() and<br>
avoid the wrapper by the same name that does nothing in common.c ?<br></blockquote><div><br>I&#39;ve put the wrapper in common.c for the sake of good order.<br></div><div>If I got it correctly, every client should have a &quot;*_make_request()&quot; function in common.c.<br>



</div><div>When compiling with optimisation, the compiler might just drop this wrapper.<br><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">




<br>
If the wrapper needs to stay for some reason, then rename send_recv() to<br>
sss_nfs_request()<br></blockquote><div><br></div><div>uhm... this is not a public function, so I&#39;m not sure the &quot;sss_nfs&quot; prefix is needed in the name.<br></div><div>also, &quot;send_recv&quot; sounds to me more descriptive of the actual action: send request *and* receive reply.<br>



</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Also use rep and rep_len, without the p, we do not change names based on<br>
whether they are pointers or not, the compiler will warn you anyway if<br>
you use them incorrectly. <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +                     const void *req, size_t req_len)<br>
&gt; +{<br>
&gt; +    int err = 0;<br>
&gt; +    enum nss_status req_rc;<br>
&gt; +    struct sss_cli_req_data rd;<br>
&gt; +    uint8_t *rep;<br>
&gt; +    size_t rep_len;<br>
&gt; +<br>
&gt; +    rd.data = req;<br>
&gt; +    rd.len = req_len;<br>
&gt; +<br>
&gt; +    sss_nss_lock();<br>
&gt; +    req_rc = sss_nfs_make_request(cmd, &amp;rd, &amp;rep, &amp;rep_len, &amp;err);<br>
&gt; +    sss_nss_unlock();<br>
&gt; +<br>
&gt; +    if (req_rc == NSS_STATUS_NOTFOUND) {<br>
&gt; +        return -ENOENT;<br>
&gt; +    }<br>
&gt; +    if (req_rc != NSS_STATUS_SUCCESS) {<br>
&gt; +        IDMAP_LOG(0, (&quot;no-make-request; err=%i (%s)&quot;, err, strerror(err)));<br>
&gt; +        return -EPIPE;<br>
&gt; +    }<br>
<br>
</div>Is -EPIPE really what rpc.idmap expects if a function fails ?<br></blockquote><div>Good point. I rechecked libnfsidmap code, and I see that I had a detail wrong.<br></div><div>libnfsidmap iterates over all plugins (in the order from the config file) and calls their translation function.<br>



</div><div>If ENOENT is returned, then iteration continues to the next plugin. on every other return value (success or other failure) the iteration will stop.<br>The proper action should be to always return 0 or ENOENT. I&#39;ll fix the code accordingly.<br>



<br><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
&gt; +    *repp = rep;<br>
&gt; +    *rep_lenp = rep_len;<br>
<br>
</div>Why the double assignment here ? Why don&#39;t you just pass the function<br>
arguments directly to sss_nfs_make_request() ?<br>
The internal function and it will not assign buffers in case of error,<br>
so you can pass in the arguments directly w/o worry.<br></blockquote><div>ok<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div><br>
&gt; +    return 0;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int get_id_from_rep(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 return_rc;<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 return_rc;<br>
&gt; +    }<br>
&gt; +    if (num_results == 0) {<br>
&gt; +        rc = -ENOENT;<br>
&gt; +        goto return_rc;<br>
&gt; +    }<br>
<br>
</div></div>You need to check that rep_len is &gt;= sizeof(uint32_t) +<br>
SSS_NFS_REPLY_ID_OFFSET before trying to copy or you could segafault if<br>
for some reason it is smaller.<br></blockquote><div><br>Right. I shouldn&#39;t have missed that.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

<div><br>
&gt; +    SAFEALIGN_COPY_UINT32(&amp;id, rep + SSS_NFS_REPLY_ID_OFFSET, NULL);<br>
&gt; +    *idp = id;<br>
&gt; +<br>
&gt; +return_rc:<br>
<br>
</div>Please use the label &#39;done&#39; when you want to exit early.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


<div><div><br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int get_name_from_rep(char *name, size_t len, uint8_t *rep,<br>
&gt; +                             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 return_rc;<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 return_rc;<br>
&gt; +    }<br>
&gt; +    if (num_results == 0) {<br>
&gt; +        rc = -ENOENT;<br>
&gt; +        goto return_rc;<br>
&gt; +    }<br>
<br>
</div></div>As above, please do boundary checks here too.<br>
<div><br>
&gt; +    buf = (const char *)(rep + SSS_NFS_REPLY_NAME_OFFSET);<br>
&gt; +    buf_len = rep_len - SSS_NFS_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; +return_rc:<br>
<br>
</div>Use &#39;done&#39;<br>
<div><br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
&gt; +/* configuration parsing aids */<br>
&gt; +static int nfs_strcasecmp(const char *s1, const char *s2)<br>
<br>
</div>You are changing the semantics of the original function here and<br>
returning a boolean that is true if the strings are equal  and false if<br>
they are not, the origin stncasecmp instead sets -1 0 or 1 depending on<br>
the comparison, by keeping a similar name you provides false<br>
expectations.<br>
<br>
Please use instead the following prototype:<br>
<br>
static bool str_equal(const char *s1, const char *s2)<br></blockquote><div>ok<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
<br>
&gt; +{<br>
&gt; +    int res = 0;<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>
<br>
<br>
</div><div>&gt; +static int nfs_conf_get_bool(char *sect, char *attr, int def)<br>
&gt; +{<br>
<br>
</div>you are getting a boolean, then please return a &#39;bool&#39;<br></blockquote><div>Generally, I prefer not to use this extension of C (but rather return 1 or 0) but if it&#39;s common practice in sssd, I can live with it.<br>
<br>

</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div><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 = (nfs_strcasecmp(&quot;1&quot;, val) ||<br>
&gt; +               nfs_strcasecmp(&quot;yes&quot;, val) ||<br>
&gt; +               nfs_strcasecmp(&quot;true&quot;, val) ||<br>
&gt; +               nfs_strcasecmp(&quot;on&quot;, val));<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return res;<br>
&gt; +}<br>
&gt; +<br>
&gt; +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
&gt; +/* The external interface */<br>
&gt; +static int sss_nfs_init(void)<br>
&gt; +{<br>
&gt; +    s_mcbuf = malloc(SSS_NFS_MCBUF_LEN);<br>
&gt; +    if (s_mcbuf == NULL) {<br>
&gt; +        return -ENOMEM;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    s_use_mc = nfs_conf_get_bool(s_sss_nfs_conf_section, s_sss_nfs_conf_use_mc,<br>
&gt; +                                 SSS_NFS_USE_MC_DEFAULT);<br>
<br>
</div></div>In what case would you want to disable using the mmap_cache ?<br></blockquote><div>Great question.<br></div><div>On servers which have both local user access (effectively meaning nss calls filling the mmap_cache) &amp; also serving NFS, the mmap cache is useful.<br>


</div><div>On contrary, with dedicated NFS servers, there are several reasons why not to use mmap_cache:<br></div><div>1. There&#39;s no local users activity (ie. no nss calls filling the mmap_cache).<br></div><div>2. The kernel has its own cache of users/groups. If the kernel requests data from rpc.idmapd then it&#39;s probably because the kernel cache was expired (and then the mmap_cache becomes useless)<br>


</div><div>3. Tthe administrator of the Linux OS might not want to configure nss to access remote directories (but rather have one-two local users) so direct communication between rpc.idmapd and sssd is in need.<br><br></div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


<div><br>
&gt; +    return 0;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int sss_nfs_princ_to_ids(char *secname, char *princ, uid_t *uid,<br>
&gt; +                                gid_t *gid, extra_mapping_params **ex)<br>
&gt; +{<br>
&gt; +    (void)secname;<br>
&gt; +    (void)princ;<br>
&gt; +    (void)uid;<br>
&gt; +    (void)gid;<br>
&gt; +    (void)ex;<br>
<br>
</div>Why these (void)s ?<br></blockquote><div>Best practice, IMHO. In case someone decided to enable &quot; -Wunused-parameter&quot; in the compile flags.<br></div><div>(I prefer that over using gcc&#39;s __attribute__(unused) )<br>


<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div><br>
&gt; +    IDMAP_LOG(0, (&quot;%s: not implemented&quot;, __func__));<br>
&gt; +    return -ENOSYS;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int sss_nfs_name_to_uid(char *name, uid_t *uid)<br>
&gt; +{<br>
&gt; +    int rc;<br>
&gt; +    size_t name_len = 0;<br>
&gt; +<br>
&gt; +    if (name == NULL) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: name is null&quot;, __func__));<br>
&gt; +        return -EINVAL;<br>
&gt; +    }<br>
&gt; +    if (uid == NULL) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: uid is null&quot;, __func__));<br>
&gt; +        return -EINVAL;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    rc = sss_strnlen(name, SSS_NAME_MAX, &amp;name_len);<br>
&gt; +    if (rc != 0) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: no-strnlen; rc=%i (%s)&quot;, __func__, rc,<br>
&gt; +                      strerror(rc)));<br>
&gt; +        return -rc;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    rc = get_uid_from_mc(uid, name);<br>
&gt; +    if (rc != 0) {<br>
&gt; +        rc = name_to_id(name, uid, SSS_NSS_GETPWNAM);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int sss_nfs_name_to_gid(char *name, gid_t *gid)<br>
&gt; +{<br>
&gt; +    int rc;<br>
&gt; +    size_t name_len = 0;<br>
&gt; +<br>
&gt; +    if (name == NULL) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: name is null&quot;, __func__));<br>
&gt; +        return -EINVAL;<br>
&gt; +    }<br>
&gt; +    if (gid == NULL) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: gid is null&quot;, __func__));<br>
&gt; +        return -EINVAL;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    rc = sss_strnlen(name, SSS_NAME_MAX, &amp;name_len);<br>
&gt; +    if (rc != 0) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: no-strnlen; rc=%i (%s)&quot;, __func__, rc,<br>
&gt; +                      strerror(rc)));<br>
&gt; +        return -rc;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    rc = get_gid_from_mc(gid, name);<br>
&gt; +    if (rc != 0) {<br>
&gt; +        rc = name_to_id(name, gid, SSS_NSS_GETGRNAM);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int sss_nfs_uid_to_name(uid_t uid, char *domain, char *name, size_t len)<br>
&gt; +{<br>
&gt; +    int rc;<br>
&gt; +<br>
&gt; +    if (name == NULL) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: name is null&quot;, __func__));<br>
&gt; +        return -EINVAL;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    rc = get_user_from_mc(name, len, uid);<br>
&gt; +    if (rc != 0) {<br>
&gt; +        rc = id_to_name(name, len, uid, SSS_NSS_GETPWUID);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int sss_nfs_gid_to_name(gid_t gid, char *domain, char *name, size_t len)<br>
&gt; +{<br>
&gt; +    int rc;<br>
&gt; +<br>
&gt; +    if (name == NULL) {<br>
&gt; +        IDMAP_LOG(0, (&quot;%s: name is null&quot;, __func__));<br>
&gt; +        return -EINVAL;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    rc = get_group_from_mc(name, len, gid);<br>
&gt; +    if (rc != 0) {<br>
&gt; +        rc = id_to_name(name, len, gid, SSS_NSS_GETGRGID);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return rc;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int sss_nfs_gss_princ_to_grouplist(<br>
&gt; +    char *secname, char *princ, gid_t *groups, int *ngroups,<br>
&gt; +    extra_mapping_params **ex)<br>
&gt; +{<br>
&gt; +    (void)secname;<br>
&gt; +    (void)princ;<br>
&gt; +    (void)groups;<br>
&gt; +    (void)ngroups;<br>
&gt; +    (void)ex;<br>
&gt; +    IDMAP_LOG(0, (&quot;%s: not implemented&quot;, __func__));<br>
&gt; +    return -ENOSYS;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static struct trans_func s_sss_nfs_trans = {<br>
&gt; +    .name = s_sss_nfs_plugin_name,<br>
&gt; +    .init = sss_nfs_init,<br>
&gt; +    .princ_to_ids = sss_nfs_princ_to_ids,<br>
&gt; +    .name_to_uid = sss_nfs_name_to_uid,<br>
&gt; +    .name_to_gid = sss_nfs_name_to_gid,<br>
&gt; +    .uid_to_name = sss_nfs_uid_to_name,<br>
&gt; +    .gid_to_name = sss_nfs_gid_to_name,<br>
&gt; +    .gss_princ_to_grouplist = sss_nfs_gss_princ_to_grouplist,<br>
&gt; +};<br>
&gt; +<br>
&gt; +struct trans_func *libnfsidmap_plugin_init(void)<br>
&gt; +{<br>
&gt; +    return (&amp;s_sss_nfs_trans);<br>
&gt; +}<br>
&gt; diff --git a/src/sss_client/sss_cli.h b/src/sss_client/sss_cli.h<br>
&gt; index 285a297..7e488e4 100644<br>
&gt; --- a/src/sss_client/sss_cli.h<br>
&gt; +++ b/src/sss_client/sss_cli.h<br>
&gt; @@ -527,6 +527,8 @@ int sss_ssh_make_request(enum sss_cli_command cmd,<br>
&gt;                           struct sss_cli_req_data *rd,<br>
&gt;                           uint8_t **repbuf, size_t *replen,<br>
&gt;                           int *errnop);<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;  #if 0<br>
&gt;<br>
<br>
<br>
--<br>
</div></div>Simo Sorce * Red Hat, Inc * New York<br>
<br>
</blockquote></div><br><br clear="all"><br>-- <br><div dir="ltr"><table cellpadding="0" cellspacing="0"><tbody><tr><td>Noam Meltzer<br>Linux Software Engineer<br>PRIMARY DATA<br><br>P.O. Box 12650, Herzliya Pituach 4673300<br>



9 Hamenofim St. Akerstein Towers, Tower A, 5th fl. Herzliya<br><br>Office: <a href="tel:%2B972-77-8981888" value="+972778981888" target="_blank">+972-77-8981888</a> | Fax: <a href="tel:%2B972-3-7617140" value="+97237617140" target="_blank">+972-3-7617140</a> | Mobile: <a href="tel:%2B972-54-5873843" value="+972545873843" target="_blank">+972-54-5873843</a><br>


<br>Email: <a href="mailto:noam@primarydata.com" target="_blank">noam@primarydata.com</a><br>
</td><td><span><label></label></span><br></td></tr></tbody></table></div>
</div></div>