<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"><<a href="mailto:simo@redhat.com" target="_blank">simo@redhat.com</a>></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't added comments. I'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>
> 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 | 490 ++++++++++++++++++++++++++++++++++++<br>
> src/sss_client/sss_cli.h | 2 +<br>
> 3 files changed, 497 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>
> 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..9f6e047<br>
> --- /dev/null<br>
> +++ b/src/sss_client/nfs/sss_nfs_client.c<br>
> @@ -0,0 +1,490 @@<br>
> +/*<br>
> + SSSD<br>
> +<br>
> + NFS Client<br>
> +<br>
> + Copyright (C) Noam Meltzer <<a href="mailto:noam@primarydata.com" target="_blank">noam@primarydata.com</a>> 2013<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>
> +#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>
> +#include "cfg.h"<br>
> +<br>
> +#include "sss_client/sss_cli.h"<br>
> +#include "sss_client/nss_mc.h"<br>
> +<br>
> +<br>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +#define SSS_NFS_PLUGIN_NAME "sss_nfs"<br>
> +#define SSS_NFS_CONF_SECTION "sss_nfs"<br>
> +#define SSS_NFS_CONF_USE_MC "memcache"<br>
> +#define SSS_NFS_REPLY_ID_OFFSET (8)<br>
> +#define SSS_NFS_REPLY_NAME_OFFSET (SSS_NFS_REPLY_ID_OFFSET + 8)<br>
> +#define SSS_NFS_MCBUF_LEN (4096)<br>
> +#define SSS_NFS_USE_MC_DEFAULT (1)<br>
> +<br>
> +<br>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +static char s_sss_nfs_plugin_name[] = SSS_NFS_PLUGIN_NAME;<br>
> +static char s_sss_nfs_conf_section[] = SSS_NFS_CONF_SECTION;<br>
> +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 "char *" (without const) where ever there's a need to pass a string. (for example, check the prototype of "conf_get_str()")<br>
</div><div>Just using a macro in these places will cause the following warning to be emitted: "initialization discards 'const' qualifier from pointer target type".<br></div><div>So I find that the cleanest approach to handle this would be to define a "static char[]" for each of these instances.<br>
<br></div><div>Now, personally I prefer to do something like this:<br></div><div>#define PLUGIN_NAME "sss_nfs"<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[] = "sss_nfs";<br><br></div><div>but I don't mind taking the second approach if that's more aligned with sssd'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>
> +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'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'll also fix the locations using "strerror" to "strerror_r".<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>
> +static int s_use_mc = SSS_NFS_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 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>
> +static int get_name_from_rep(char *name, size_t len, uint8_t *rep,<br>
> + 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>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +/* get from memcache functions */<br>
> +static int get_uid_from_mc(id_t *uid, const char *name)<br>
> +{<br>
> + int rc;<br>
> + struct passwd pwd;<br>
> + char *buf;<br>
<br>
</div>Use char buf[SSS_NFS_MCBUF_LEN]; here instead of a globally allocate<br>
buffer.<br>
<br>
It'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>
> + size_t len = 0;<br>
> +<br>
> + if (!s_use_mc) {<br>
> + return -1;<br>
> + }<br>
> +<br>
> + buf = s_mcbuf;<br>
> + sss_strnlen(name, SSS_NAME_MAX, &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 "entry" 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 "sss_strnlen()" 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>
> + rc = sss_nss_mc_getpwnam(name, len, &pwd, buf, SSS_NFS_MCBUF_LEN);<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>
> + return rc;<br>
> +}<br>
> +<br>
> +static int get_gid_from_mc(id_t *gid, const char *name) {<br>
> + int rc;<br>
> + struct group grp;<br>
> + char *buf;<br>
> + size_t len;<br>
> +<br>
> + if (!s_use_mc) {<br>
> + return -1;<br>
> + }<br>
> +<br>
> + buf = s_mcbuf;<br>
> + sss_strnlen(name, SSS_NAME_MAX, &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>
> + rc = sss_nss_mc_getgrnam(name, len, &grp, buf, SSS_NFS_MCBUF_LEN);<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>
> + return rc;<br>
> +}<br>
> +<br>
> +static int get_user_from_mc(char *name, size_t len, uid_t uid)<br>
> +{<br>
> + int rc;<br>
> + struct passwd pwd;<br>
> + char *buf;<br>
> + 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>
> + if (!s_use_mc) {<br>
> + return -1;<br>
> + }<br>
> +<br>
> + 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>
> + rc = sss_nss_mc_getpwuid(uid, &pwd, buf, SSS_NFS_MCBUF_LEN);<br>
> + if (rc == 0) {<br>
> + cache_len = strlen(pwd.pw_name) + 1;<br>
> + if (cache_len > len) {<br>
> + IDMAP_LOG(0, ("%s: reply too long; cache_len=%lu, len=%lu",<br>
> + __func__, cache_len, len));<br>
> + rc = -ENOBUFS;<br>
> + }<br>
> + memcpy(name, pwd.pw_name, cache_len);<br>
> + }<br>
> +<br>
> + return rc;<br>
> +}<br>
> +<br>
> +static int get_group_from_mc(char *name, size_t len, id_t gid)<br>
> +{<br>
> + int rc;<br>
> + struct group grp;<br>
> + char *buf;<br>
> + size_t cache_len;<br>
> +<br>
> + 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>
> + rc = sss_nss_mc_getgrgid(gid, &grp, buf, SSS_NFS_MCBUF_LEN);<br>
> + if (rc == 0) {<br>
> + cache_len = strlen(grp.gr_name) + 1;<br>
> + if (cache_len > len) {<br>
> + IDMAP_LOG(0, ("%s: reply too long; cache_len=%lu, len=%lu",<br>
> + __func__, cache_len, len));<br>
> + rc = -ENOBUFS;<br>
> + }<br>
> + memcpy(name, grp.gr_name, cache_len);<br>
> + }<br>
> +<br>
> + return rc;<br>
> +}<br>
> +<br>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +static int name_to_id(const char *name, id_t *id, enum sss_cli_command cmd)<br>
> +{<br>
> + int rc;<br>
> + uint8_t *rep = NULL;<br>
> + size_t rep_len = 0;<br>
> + size_t name_len;<br>
> +<br>
> + sss_strnlen(name, SSS_NAME_MAX, &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>
> + rc = send_recv(&rep, &rep_len, cmd, name, name_len + 1);<br>
> + if (rc == 0) {<br>
> + rc = get_id_from_rep(id, rep, rep_len);<br>
> + }<br>
> +<br>
> + if (rep != NULL) {<br>
<br>
</div>Don'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>
> + free(rep);<br>
> + }<br>
> +<br>
> + return rc;<br>
> +}<br>
> +<br>
> +static int id_to_name(char *name, size_t len, id_t id,<br>
> + enum sss_cli_command cmd)<br>
> +{<br>
> + int rc;<br>
> + const void *req = NULL;<br>
> + uint8_t *rep = NULL;<br>
> + size_t req_len;<br>
> + size_t rep_len = 0;<br>
> +<br>
> + req = (void *)(&id);<br>
> + 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, &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>
> + rc = send_recv(&rep, &rep_len, cmd, req, req_len);<br>
> + if (rc == 0) {<br>
> + rc = get_name_from_rep(name, len, rep, rep_len);<br>
> + }<br>
> +<br>
> + if (rep != NULL) {<br>
> + free(rep);<br>
> + }<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>
> +<br>
> + return rc;<br>
> +}<br>
> +<br>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +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'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 "*_make_request()" 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'm not sure the "sss_nfs" prefix is needed in the name.<br></div><div>also, "send_recv" 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>
> + const void *req, size_t req_len)<br>
> +{<br>
> + int err = 0;<br>
> + enum nss_status req_rc;<br>
> + struct sss_cli_req_data rd;<br>
> + uint8_t *rep;<br>
> + size_t rep_len;<br>
> +<br>
> + rd.data = req;<br>
> + rd.len = req_len;<br>
> +<br>
> + sss_nss_lock();<br>
> + req_rc = sss_nfs_make_request(cmd, &rd, &rep, &rep_len, &err);<br>
> + sss_nss_unlock();<br>
> +<br>
> + if (req_rc == NSS_STATUS_NOTFOUND) {<br>
> + return -ENOENT;<br>
> + }<br>
> + if (req_rc != NSS_STATUS_SUCCESS) {<br>
> + IDMAP_LOG(0, ("no-make-request; err=%i (%s)", err, strerror(err)));<br>
> + return -EPIPE;<br>
> + }<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'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>
> + *repp = rep;<br>
> + *rep_lenp = rep_len;<br>
<br>
</div>Why the double assignment here ? Why don'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>
> + return 0;<br>
> +}<br>
> +<br>
> +static int get_id_from_rep(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 return_rc;<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 return_rc;<br>
> + }<br>
> + if (num_results == 0) {<br>
> + rc = -ENOENT;<br>
> + goto return_rc;<br>
> + }<br>
<br>
</div></div>You need to check that rep_len is >= 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'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>
> + SAFEALIGN_COPY_UINT32(&id, rep + SSS_NFS_REPLY_ID_OFFSET, NULL);<br>
> + *idp = id;<br>
> +<br>
> +return_rc:<br>
<br>
</div>Please use the label 'done' 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>
> + return rc;<br>
> +}<br>
> +<br>
> +static int get_name_from_rep(char *name, size_t len, uint8_t *rep,<br>
> + 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 return_rc;<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 return_rc;<br>
> + }<br>
> + if (num_results == 0) {<br>
> + rc = -ENOENT;<br>
> + goto return_rc;<br>
> + }<br>
<br>
</div></div>As above, please do boundary checks here too.<br>
<div><br>
> + buf = (const char *)(rep + SSS_NFS_REPLY_NAME_OFFSET);<br>
> + buf_len = rep_len - SSS_NFS_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>
> +return_rc:<br>
<br>
</div>Use 'done'<br>
<div><br>
> + return rc;<br>
> +}<br>
> +<br>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +/* configuration parsing aids */<br>
> +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>
> +{<br>
> + int res = 0;<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>
<br>
</div><div>> +static int nfs_conf_get_bool(char *sect, char *attr, int def)<br>
> +{<br>
<br>
</div>you are getting a boolean, then please return a 'bool'<br></blockquote><div>Generally, I prefer not to use this extension of C (but rather return 1 or 0) but if it'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>
> + int res;<br>
> + char *val;<br>
> +<br>
> + res = def;<br>
> + val = conf_get_str(sect, attr);<br>
> + if (val) {<br>
> + res = (nfs_strcasecmp("1", val) ||<br>
> + nfs_strcasecmp("yes", val) ||<br>
> + nfs_strcasecmp("true", val) ||<br>
> + nfs_strcasecmp("on", val));<br>
> + }<br>
> +<br>
> + return res;<br>
> +}<br>
> +<br>
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/<br>
> +/* The external interface */<br>
> +static int sss_nfs_init(void)<br>
> +{<br>
> + s_mcbuf = malloc(SSS_NFS_MCBUF_LEN);<br>
> + if (s_mcbuf == NULL) {<br>
> + return -ENOMEM;<br>
> + }<br>
> +<br>
> + s_use_mc = nfs_conf_get_bool(s_sss_nfs_conf_section, s_sss_nfs_conf_use_mc,<br>
> + 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) & 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'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'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>
> + return 0;<br>
> +}<br>
> +<br>
> +static int sss_nfs_princ_to_ids(char *secname, char *princ, uid_t *uid,<br>
> + gid_t *gid, extra_mapping_params **ex)<br>
> +{<br>
> + (void)secname;<br>
> + (void)princ;<br>
> + (void)uid;<br>
> + (void)gid;<br>
> + (void)ex;<br>
<br>
</div>Why these (void)s ?<br></blockquote><div>Best practice, IMHO. In case someone decided to enable " -Wunused-parameter" in the compile flags.<br></div><div>(I prefer that over using gcc'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>
> + IDMAP_LOG(0, ("%s: not implemented", __func__));<br>
> + return -ENOSYS;<br>
> +}<br>
> +<br>
> +static int sss_nfs_name_to_uid(char *name, uid_t *uid)<br>
> +{<br>
> + int rc;<br>
> + size_t name_len = 0;<br>
> +<br>
> + if (name == NULL) {<br>
> + IDMAP_LOG(0, ("%s: name is null", __func__));<br>
> + return -EINVAL;<br>
> + }<br>
> + if (uid == NULL) {<br>
> + IDMAP_LOG(0, ("%s: uid is null", __func__));<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + rc = sss_strnlen(name, SSS_NAME_MAX, &name_len);<br>
> + if (rc != 0) {<br>
> + IDMAP_LOG(0, ("%s: no-strnlen; rc=%i (%s)", __func__, rc,<br>
> + strerror(rc)));<br>
> + return -rc;<br>
> + }<br>
> +<br>
> + rc = get_uid_from_mc(uid, name);<br>
> + if (rc != 0) {<br>
> + rc = name_to_id(name, uid, SSS_NSS_GETPWNAM);<br>
> + }<br>
> +<br>
> + return rc;<br>
> +}<br>
> +<br>
> +static int sss_nfs_name_to_gid(char *name, gid_t *gid)<br>
> +{<br>
> + int rc;<br>
> + size_t name_len = 0;<br>
> +<br>
> + if (name == NULL) {<br>
> + IDMAP_LOG(0, ("%s: name is null", __func__));<br>
> + return -EINVAL;<br>
> + }<br>
> + if (gid == NULL) {<br>
> + IDMAP_LOG(0, ("%s: gid is null", __func__));<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + rc = sss_strnlen(name, SSS_NAME_MAX, &name_len);<br>
> + if (rc != 0) {<br>
> + IDMAP_LOG(0, ("%s: no-strnlen; rc=%i (%s)", __func__, rc,<br>
> + strerror(rc)));<br>
> + return -rc;<br>
> + }<br>
> +<br>
> + rc = get_gid_from_mc(gid, name);<br>
> + if (rc != 0) {<br>
> + rc = name_to_id(name, gid, SSS_NSS_GETGRNAM);<br>
> + }<br>
> +<br>
> + return rc;<br>
> +}<br>
> +<br>
> +static int sss_nfs_uid_to_name(uid_t uid, char *domain, char *name, size_t len)<br>
> +{<br>
> + int rc;<br>
> +<br>
> + if (name == NULL) {<br>
> + IDMAP_LOG(0, ("%s: name is null", __func__));<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + rc = get_user_from_mc(name, len, uid);<br>
> + if (rc != 0) {<br>
> + rc = id_to_name(name, len, uid, SSS_NSS_GETPWUID);<br>
> + }<br>
> +<br>
> + return rc;<br>
> +}<br>
> +<br>
> +static int sss_nfs_gid_to_name(gid_t gid, char *domain, char *name, size_t len)<br>
> +{<br>
> + int rc;<br>
> +<br>
> + if (name == NULL) {<br>
> + IDMAP_LOG(0, ("%s: name is null", __func__));<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + rc = get_group_from_mc(name, len, gid);<br>
> + if (rc != 0) {<br>
> + rc = id_to_name(name, len, gid, SSS_NSS_GETGRGID);<br>
> + }<br>
> +<br>
> + return rc;<br>
> +}<br>
> +<br>
> +static int sss_nfs_gss_princ_to_grouplist(<br>
> + char *secname, char *princ, gid_t *groups, int *ngroups,<br>
> + extra_mapping_params **ex)<br>
> +{<br>
> + (void)secname;<br>
> + (void)princ;<br>
> + (void)groups;<br>
> + (void)ngroups;<br>
> + (void)ex;<br>
> + IDMAP_LOG(0, ("%s: not implemented", __func__));<br>
> + return -ENOSYS;<br>
> +}<br>
> +<br>
> +static struct trans_func s_sss_nfs_trans = {<br>
> + .name = s_sss_nfs_plugin_name,<br>
> + .init = sss_nfs_init,<br>
> + .princ_to_ids = sss_nfs_princ_to_ids,<br>
> + .name_to_uid = sss_nfs_name_to_uid,<br>
> + .name_to_gid = sss_nfs_name_to_gid,<br>
> + .uid_to_name = sss_nfs_uid_to_name,<br>
> + .gid_to_name = sss_nfs_gid_to_name,<br>
> + .gss_princ_to_grouplist = sss_nfs_gss_princ_to_grouplist,<br>
> +};<br>
> +<br>
> +struct trans_func *libnfsidmap_plugin_init(void)<br>
> +{<br>
> + return (&s_sss_nfs_trans);<br>
> +}<br>
> diff --git a/src/sss_client/sss_cli.h b/src/sss_client/sss_cli.h<br>
> index 285a297..7e488e4 100644<br>
> --- a/src/sss_client/sss_cli.h<br>
> +++ b/src/sss_client/sss_cli.h<br>
> @@ -527,6 +527,8 @@ int sss_ssh_make_request(enum sss_cli_command cmd,<br>
> struct sss_cli_req_data *rd,<br>
> uint8_t **repbuf, size_t *replen,<br>
> int *errnop);<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>
> #if 0<br>
><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>