<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"><<a href="mailto:jhrozek@redhat.com" target="_blank">jhrozek@redhat.com</a>></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>
> Implementation of design document:<br>
> <a href="https://fedorahosted.org/sssd/wiki/DesignDocs/rpc.idmapd%20plugin" target="_blank">https://fedorahosted.org/sssd/wiki/DesignDocs/rpc.idmapd%20plugin</a><br>
> ---<br>
> src/sss_client/common.c | 5 +<br>
> src/sss_client/nfs/sss_nfs_client.c | 569 ++++++++++++++++++++++++++++++++++++<br>
> src/sss_client/sss_cli.h | 2 +<br>
> 3 files changed, 576 insertions(+)<br>
> create mode 100644 src/sss_client/nfs/sss_nfs_client.c<br>
><br>
> diff --git a/src/sss_client/common.c b/src/sss_client/common.c<br>
> index 6044af0..58a9eca 100644<br>
> --- a/src/sss_client/common.c<br>
> +++ b/src/sss_client/common.c<br>
> @@ -936,6 +936,11 @@ int sss_ssh_make_request(enum sss_cli_command cmd,<br>
> return ret;<br>
> }<br>
><br>
> +int sss_nfs_make_request(enum sss_cli_command cmd, struct sss_cli_req_data *rd,<br>
> + uint8_t **rep, size_t *replen, int *errnop)<br>
> +{<br>
> + return sss_nss_make_request(cmd, rd, rep, replen, errnop);<br>
> +}<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'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 "simple" wrapper for a single reason - code readability.</div><div>at the end, I believe the compiler will "optimise out" this function. (though, I haven't actually tested it)</div>
<div>bottom line - let me know what you prefer, and I'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>
><br>
> const char *ssscli_err2string(int err)<br>
> {<br>
> diff --git a/src/sss_client/nfs/sss_nfs_client.c b/src/sss_client/nfs/sss_nfs_client.c<br>
> new file mode 100644<br>
> index 0000000..7b23eab<br>
> --- /dev/null<br>
> +++ b/src/sss_client/nfs/sss_nfs_client.c<br>
> @@ -0,0 +1,569 @@<br>
> +/*<br>
> + SSSD<br>
> +<br>
> + NFS Client<br>
> +<br>
> + Copyright (C) Noam Meltzer <<a href="mailto:noam@primarydata.com">noam@primarydata.com</a>> 2013-2014<br>
<br>
</div>This, and other places -- I guess you'd like to update the address to<br>
something that exists :-)<br></blockquote><div>PrimaryData deserves their credit. I'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>
> +<br>
> + This program is free software; you can redistribute it and/or modify<br>
> + it under the terms of the GNU General Public License as published by<br>
> + the Free Software Foundation; either version 3 of the License, or<br>
> + (at your option) any later version.<br>
> +<br>
> + This program is distributed in the hope that it will be useful,<br>
> + but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the<br>
> + GNU General Public License for more details.<br>
> +<br>
> + You should have received a copy of the GNU General Public License<br>
> + along with this program. If not, see <<a href="http://www.gnu.org/licenses/" target="_blank">http://www.gnu.org/licenses/</a>>.<br>
> +*/<br>
> +<br>
> +#define _GNU_SOURCE<br>
> +<br>
> +#include <stddef.h><br>
> +#include <stdlib.h><br>
> +#include <sys/types.h><br>
> +#include <errno.h><br>
> +#include <string.h><br>
> +<br>
> +#include <nfsidmap.h><br>
> +#include "nfsidmap_internal.h"<br>
> +<br>
> +#include "sss_client/sss_cli.h"<br>
> +#include "sss_client/nss_mc.h"<br>
> +<br>
> +<br>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +#define PLUGIN_NAME "sss_nfs"<br>
> +#define CONF_SECTION "sss_nfs"<br>
> +#define CONF_USE_MC "memcache"<br>
> +#define REPLY_ID_OFFSET (8)<br>
> +#define REPLY_NAME_OFFSET (REPLY_ID_OFFSET + 8)<br>
> +#define MCBUF_LEN (4096)<br>
<br>
</div></div>I think this constant is a bit misnamed. The MC prefix suggests that<br>
it's only used for memory cache results, which is not true.<br></blockquote><div><br></div><div>Will "BUF_LEN" 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>
> +#define USE_MC_DEFAULT true<br>
> +<br>
> +<br>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +static char sss_nfs_plugin_name[] = PLUGIN_NAME;<br>
> +static char nfs_conf_sect[] = CONF_SECTION;<br>
> +static char nfs_conf_use_mc[] = CONF_USE_MC;<br>
> +<br>
> +static bool nfs_use_mc = USE_MC_DEFAULT;<br>
> +<br>
> +<br>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +/* Forward declarations */<br>
> +static int send_recv(uint8_t **repp, size_t *rep_lenp, enum sss_cli_command cmd,<br>
> + const void *req, size_t req_len);<br>
> +static int reply_to_id(id_t *idp, uint8_t *rep, size_t rep_len);<br>
> +static int reply_to_name(char *name, size_t len, uint8_t *rep, size_t rep_len);<br>
> +<br>
> +<br>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +/* get from memcache functions */<br>
> +static int get_uid_from_mc(id_t *uid, const char *name)<br>
> +{<br>
> + int rc = 0;<br>
> + struct passwd pwd;<br>
> + char *buf = NULL;<br>
> + char *p = NULL;<br>
> + size_t buflen = 0;<br>
> + size_t len = 0;<br>
> +<br>
> + if (!nfs_use_mc) {<br>
> + return -1;<br>
> + }<br>
> +<br>
> + sss_strnlen(name, SSS_NAME_MAX, &len);<br>
> +<br>
> + do {<br>
> + buflen += MCBUF_LEN;<br>
> + if ((p = realloc(buf, buflen)) == NULL) {<br>
> + rc = ENOMEM;<br>
> + goto done;<br>
> + }<br>
> + buf = p;<br>
> + rc = sss_nss_mc_getpwnam(name, len, &pwd, buf, buflen);<br>
> + } while (rc == ERANGE);<br>
> +<br>
> + if (rc == 0) {<br>
> + IDMAP_LOG(1, ("found user %s in memcache", name));<br>
> + *uid = pwd.pw_uid;<br>
> + } else {<br>
> + IDMAP_LOG(1, ("user %s not in memcache", name));<br>
> + }<br>
> +<br>
> +done:<br>
> + free(buf);<br>
> + return rc;<br>
> +}<br>
> +<br>
> +static int get_gid_from_mc(id_t *gid, const char *name) {<br>
</div></div> ^^^^^^^^<br>
We prefer to put the opening "{" 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>
> + int rc = 0;<br>
> + struct group grp;<br>
> + char *buf = NULL;<br>
> + char *p = NULL;<br>
> + size_t buflen = 0;<br>
> + size_t len;<br>
> +<br>
> + if (!nfs_use_mc) {<br>
> + return -1;<br>
> + }<br>
> +<br>
> + sss_strnlen(name, SSS_NAME_MAX, &len);<br>
> +<br>
> + do {<br>
> + buflen += MCBUF_LEN;<br>
> + if ((p = realloc(buf, buflen)) == NULL) {<br>
> + rc = ENOMEM;<br>
> + goto done;<br>
> + }<br>
> + buf = p;<br>
> + rc = sss_nss_mc_getgrnam(name, len, &grp, buf, buflen);<br>
> + } while (rc == ERANGE);<br>
> +<br>
> + if (rc == 0) {<br>
> + IDMAP_LOG(1, ("found group %s in memcache", name));<br>
> + *gid = grp.gr_gid;<br>
> + } else {<br>
> + IDMAP_LOG(1, ("group %s not in memcache", name));<br>
> + }<br>
> +<br>
> +done:<br>
> + free(buf);<br>
> + return rc;<br>
> +}<br>
<br>
</div></div>[snip]<br>
<div><div class="h5"><br>
> +static int reply_to_id(id_t *idp, uint8_t *rep, size_t rep_len)<br>
> +{<br>
> + int rc = 0;<br>
> + id_t id;<br>
> + uint32_t num_results = 0;<br>
> +<br>
> + if (rep_len < sizeof(uint32_t)) {<br>
> + IDMAP_LOG(0, ("%s: reply too small; rep_len=%lu", __func__, rep_len));<br>
> + rc = EBADMSG;<br>
> + goto done;<br>
> + }<br>
> +<br>
> + SAFEALIGN_COPY_UINT32(&num_results, rep, NULL);<br>
> + if (num_results > 1) {<br>
> + IDMAP_LOG(0, ("%s: too many results (%lu)", __func__, num_results));<br>
> + rc = EBADMSG;<br>
> + goto done;<br>
> + }<br>
> + if (num_results == 0) {<br>
> + rc = ENOENT;<br>
> + goto done;<br>
> + }<br>
> + if (rep_len < sizeof(uint32_t) + REPLY_ID_OFFSET) {<br>
> + IDMAP_LOG(0, ("%s: reply too small(2); rep_len=%lu", __func__,<br>
> + rep_len));<br>
> + rc = EBADMSG;<br>
> + goto done;<br>
> + }<br>
<br>
</div></div>Could we use just this ^ check instead of the one at the beginning of<br>
the function? It's not like any partial results can be used if the<br>
buffer is > sizeof(uint32_t) but < sizeof(uint32_t) + REPLY_ID_OFFSET.<br>
<div><div class="h5"><br></div></div></blockquote><div>the first check ( rep_len < sizeof(uint32_t) ) is mainly defensive programming: what if rep_len is 0?</div><div>if i'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">
> +<br>
> + SAFEALIGN_COPY_UINT32(&id, rep + REPLY_ID_OFFSET, NULL);<br>
> + *idp = id;<br>
> +<br>
> +done:<br>
> + return rc;<br>
> +}<br>
> +<br>
> +static int reply_to_name(char *name, size_t len, uint8_t *rep, size_t rep_len)<br>
> +{<br>
> + int rc = 0;<br>
> + uint32_t num_results = 0;<br>
> + const char *buf;<br>
> + size_t buf_len;<br>
> + size_t offset;<br>
> +<br>
> + if (rep_len < sizeof(uint32_t)) {<br>
> + IDMAP_LOG(0, ("%s: reply too small; rep_len=%lu", __func__, rep_len));<br>
> + rc = EBADMSG;<br>
> + goto done;<br>
> + }<br>
> +<br>
> + SAFEALIGN_COPY_UINT32(&num_results, rep, NULL);<br>
> + if (num_results > 1) {<br>
> + IDMAP_LOG(0, ("%s: too many results (%lu)", __func__, num_results));<br>
> + rc = EBADMSG;<br>
> + goto done;<br>
> + }<br>
> + if (num_results == 0) {<br>
> + rc = ENOENT;<br>
> + goto done;<br>
> + }<br>
> + if (rep_len < sizeof(uint32_t) + REPLY_ID_OFFSET) {<br>
<br>
</div></div>I think this is a typo, shouldn'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>
> + IDMAP_LOG(0, ("%s: reply too small(2); rep_len=%lu", __func__,<br>
> + rep_len));<br>
> + rc = EBADMSG;<br>
> + goto done;<br>
> + }<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>
> +<br>
> + buf = (const char *)(rep + REPLY_NAME_OFFSET);<br>
> + buf_len = rep_len - REPLY_NAME_OFFSET;<br>
> + offset = 0;<br>
> + rc = sss_readrep_copy_string(buf, &offset, &buf_len, &len, &name, NULL);<br>
> + if (rc != 0) {<br>
> + rc = -rc;<br>
> + }<br>
> +<br>
> +done:<br>
> + return rc;<br>
> +}<br>
> +<br>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +/* configuration parsing aids */<br>
> +static bool str_equal(const char *s1, const char *s2)<br>
> +{<br>
> + bool res = false;<br>
> + size_t len1;<br>
> + size_t len2;<br>
> +<br>
> + len1 = strlen(s1);<br>
> + len2 = strlen(s2);<br>
> +<br>
> + if (len1 == len2) {<br>
> + res = (strncasecmp(s1, s2, len1) == 0);<br>
> + }<br>
> +<br>
> + return res;<br>
> +}<br>
> +<br>
> +static int nfs_conf_get_bool(char *sect, char *attr, int def)<br>
> +{<br>
> + int res;<br>
> + char *val;<br>
> +<br>
> + res = def;<br>
> + val = conf_get_str(sect, attr);<br>
> + if (val) {<br>
> + res = (str_equal("1", val) ||<br>
> + str_equal("yes", val) ||<br>
> + str_equal("true", val) ||<br>
> + str_equal("on", val));<br>
> + }<br>
> +<br>
> + return res;<br>
> +}<br>
> +<br>
> +<br>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +/* libnfsidmap return-code aids */<br>
> +<br>
> +/*<br>
> + * we only want to return 0 or ENOENT; otherwise libnfsidmap will stop<br>
> + * translation instead of proceeding to the next translation plugin<br>
> + */<br>
> +int normalise_rc(int rc) {<br>
</div></div> ^^^^^^^<br>
Same comment like get_gid_from_mc<br>
<div class="">> + int res;<br>
> +<br>
> + res = rc;<br>
> + if (res != 0 && res != ENOENT) {<br>
> + res = ENOENT;<br>
> + }<br>
> +<br>
> + return res;<br>
> +}<br>
> +<br>
> +/* log the actual rc from our code (to be used before normalising the rc) */<br>
> +void log_actual_rc(const char *trans_name, int rc) {<br>
</div> ^^^^^^^^^<br>
And here.<br>
<div class="">> + char tmp[80];<br>
> + IDMAP_LOG(1, ("%s: rc=%i msg=%s", trans_name, rc,<br>
> + strerror_r(rc, tmp, sizeof(tmp))));<br>
> +}<br>
> +<br>
> +<br>
<br>
</div>The rest of the file looks good to me.<br>
</blockquote></div><br></div></div>