On Tue, Mar 04, 2014 at 09:37:52AM +0200, Noam Meltzer wrote:I wonder if we need to add this call at all. Since the pipe we are
> 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);
> +}
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?
This, and other places -- I guess you'd like to update the address to
>
> 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
something that exists :-)
I think this constant is a bit misnamed. The MC prefix suggests that
> +
> + 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)
it's only used for memory cache results, which is not true.
^^^^^^^^
> +#define USE_MC_DEFAULT true
> +
> +
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
> +static char sss_nfs_plugin_name[] = PLUGIN_NAME;
> +static char nfs_conf_sect[] = CONF_SECTION;
> +static char nfs_conf_use_mc[] = CONF_USE_MC;
> +
> +static bool nfs_use_mc = USE_MC_DEFAULT;
> +
> +
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
> +/* Forward declarations */
> +static int send_recv(uint8_t **repp, size_t *rep_lenp, enum sss_cli_command cmd,
> + const void *req, size_t req_len);
> +static int reply_to_id(id_t *idp, uint8_t *rep, size_t rep_len);
> +static int reply_to_name(char *name, size_t len, uint8_t *rep, size_t rep_len);
> +
> +
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
> +/* get from memcache functions */
> +static int get_uid_from_mc(id_t *uid, const char *name)
> +{
> + int rc = 0;
> + struct passwd pwd;
> + char *buf = NULL;
> + char *p = NULL;
> + size_t buflen = 0;
> + size_t len = 0;
> +
> + if (!nfs_use_mc) {
> + return -1;
> + }
> +
> + sss_strnlen(name, SSS_NAME_MAX, &len);
> +
> + do {
> + buflen += MCBUF_LEN;
> + if ((p = realloc(buf, buflen)) == NULL) {
> + rc = ENOMEM;
> + goto done;
> + }
> + buf = p;
> + rc = sss_nss_mc_getpwnam(name, len, &pwd, buf, buflen);
> + } while (rc == ERANGE);
> +
> + if (rc == 0) {
> + IDMAP_LOG(1, ("found user %s in memcache", name));
> + *uid = pwd.pw_uid;
> + } else {
> + IDMAP_LOG(1, ("user %s not in memcache", name));
> + }
> +
> +done:
> + free(buf);
> + return rc;
> +}
> +
> +static int get_gid_from_mc(id_t *gid, const char *name) {
We prefer to put the opening "{" after a newline for function
declarations.
[snip]
> + int rc = 0;
> + struct group grp;
> + char *buf = NULL;
> + char *p = NULL;
> + size_t buflen = 0;
> + size_t len;
> +
> + if (!nfs_use_mc) {
> + return -1;
> + }
> +
> + sss_strnlen(name, SSS_NAME_MAX, &len);
> +
> + do {
> + buflen += MCBUF_LEN;
> + if ((p = realloc(buf, buflen)) == NULL) {
> + rc = ENOMEM;
> + goto done;
> + }
> + buf = p;
> + rc = sss_nss_mc_getgrnam(name, len, &grp, buf, buflen);
> + } while (rc == ERANGE);
> +
> + if (rc == 0) {
> + IDMAP_LOG(1, ("found group %s in memcache", name));
> + *gid = grp.gr_gid;
> + } else {
> + IDMAP_LOG(1, ("group %s not in memcache", name));
> + }
> +
> +done:
> + free(buf);
> + return rc;
> +}
Could we use just this ^ check instead of the one at the beginning of
> +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;
> + }
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.
I think this is a typo, shouldn't the length be checked against> +
> + SAFEALIGN_COPY_UINT32(&id, rep + REPLY_ID_OFFSET, NULL);
> + *idp = id;
> +
> +done:
> + return rc;
> +}
> +
> +static int reply_to_name(char *name, size_t len, uint8_t *rep, size_t rep_len)
> +{
> + int rc = 0;
> + uint32_t num_results = 0;
> + const char *buf;
> + size_t buf_len;
> + size_t offset;
> +
> + 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) {
REPLY_NAME_OFFSET instead?
Similar comment about the length check.
> + IDMAP_LOG(0, ("%s: reply too small(2); rep_len=%lu", __func__,
> + rep_len));
> + rc = EBADMSG;
> + goto done;
> + }
^^^^^^^
> +
> + buf = (const char *)(rep + REPLY_NAME_OFFSET);
> + buf_len = rep_len - REPLY_NAME_OFFSET;
> + offset = 0;
> + rc = sss_readrep_copy_string(buf, &offset, &buf_len, &len, &name, NULL);
> + if (rc != 0) {
> + rc = -rc;
> + }
> +
> +done:
> + return rc;
> +}
> +
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
> +/* configuration parsing aids */
> +static bool str_equal(const char *s1, const char *s2)
> +{
> + bool res = false;
> + size_t len1;
> + size_t len2;
> +
> + len1 = strlen(s1);
> + len2 = strlen(s2);
> +
> + if (len1 == len2) {
> + res = (strncasecmp(s1, s2, len1) == 0);
> + }
> +
> + return res;
> +}
> +
> +static int nfs_conf_get_bool(char *sect, char *attr, int def)
> +{
> + int res;
> + char *val;
> +
> + res = def;
> + val = conf_get_str(sect, attr);
> + if (val) {
> + res = (str_equal("1", val) ||
> + str_equal("yes", val) ||
> + str_equal("true", val) ||
> + str_equal("on", val));
> + }
> +
> + return res;
> +}
> +
> +
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
> +/* libnfsidmap return-code aids */
> +
> +/*
> + * we only want to return 0 or ENOENT; otherwise libnfsidmap will stop
> + * translation instead of proceeding to the next translation plugin
> + */
> +int normalise_rc(int rc) {
Same comment like get_gid_from_mc
> + int res;^^^^^^^^^
> +
> + res = rc;
> + if (res != 0 && res != ENOENT) {
> + res = ENOENT;
> + }
> +
> + return res;
> +}
> +
> +/* log the actual rc from our code (to be used before normalising the rc) */
> +void log_actual_rc(const char *trans_name, int rc) {
And here.
> + char tmp[80];The rest of the file looks good to me.
> + IDMAP_LOG(1, ("%s: rc=%i msg=%s", trans_name, rc,
> + strerror_r(rc, tmp, sizeof(tmp))));
> +}
> +
> +