[SSSD] [PATCH] Check NSCD configuration file

Jakub Hrozek jhrozek at redhat.com
Fri May 17 08:29:28 UTC 2013


On Tue, May 14, 2013 at 06:32:35PM +0200, Ondrej Kos wrote:
> On 05/14/2013 04:44 PM, Jakub Hrozek wrote:
> >On Mon, May 13, 2013 at 05:00:42PM +0200, Ondrej Kos wrote:
> >>Hi,
> >>
> >>Attached is patch for the following trac issue
> >>https://fedorahosted.org/sssd/ticket/1785
> >>
> >>Please feel free to correct my wording of the syslog messages.
> >
> >See some comments inline.
> >
> >>--- a/configure.ac
> >>+++ b/configure.ac
> >>@@ -166,6 +166,10 @@ fi
> >>
> >>  WITH_LIBNL
> >>
> >>+if test x$have_nscd; then
> >>+    WITH_NSCD_CONF
> >>+fi
> >>+
> >
> >Where does $have_nscd come from? git grep have_nscd returns nothing.
> 
> Should've been in capitals
> 
> src/conf_macros.m4:352:    AC_DEFINE_UNQUOTED(HAVE_NSCD, $NSCD_PATH, [flush
> nscd cache after local domain operations])

This is much better and I only see some nitpicks now.

I discussed the issue yesterday on IRC with Stephen as well and he
suggested that in some cases running nscd along with SSSD might be
beneficial to avoid opening /etc/passwd on each getpwnam. But the
problem is that nscd can only cache the whole database, not per-source.
So I opened https://bugzilla.redhat.com/show_bug.cgi?id=963908 can you
reference that bz in the commit message and add a comment to the code that
this is a hack until we can configure nscd better?

> +            case EOK:
> +                DEBUG(SSSDBG_TRACE_FUNC, ("NSCD socket was detected and it "
> +                            "seems to be configured not to interfere with "
> +                            "SSSD's caching capabilities\n"));
> +        }

Indentation is off here.

>      }
>  
>      /* Parse config file, fail if cannot be done */
> diff --git a/src/util/util.c b/src/util/util.c
> index ba85e0da2a2e03784c5bb9abbbd038ddaeb3455c..e48c94f36023fef621fbe19ff581c42913acf84f 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -688,3 +688,108 @@ void safezero(void *data, size_t size)
>          *p++ = 0;
>      }
>  }
> +
> +/* NSCD config file parse and check */
> +
> +struct sss_nscd_db {
> +    const char *svc_type_name;
> +    unsigned int nscd_service_flag;
> +};
> +

The structure definition can be moved inside sss_nscd_check_service(),
it's only ever going to be used in that function.

> +unsigned int sss_nscd_check_service(char* svc_name)
> +{

Please make the function static.

> +    int i;
> +    unsigned int ret = 0;
> +
> +    struct sss_nscd_db db[] = {
> +        { "passwd",   0x0001 },
> +        { "group",    0x0010 },
> +        { "netgroup", 0x0100 },
> +        { "services", 0x1000 },
> +        { NULL,       0   }
> +    };
> +
> +    for (i = 0; db[i].svc_type_name != NULL; i++) {
> +        if (!strcmp(db[i].svc_type_name, svc_name)) {
> +
> +            ret |= db[i].nscd_service_flag;

You can just assign here, no need to OR the result. It's going to be
just one number anyway.

> +            break;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +errno_t sss_nscd_parse_conf(void)
> +{
> +    FILE *fp;
> +    unsigned int occured = 0;
> +    char *line, *part1, *part2, *part3, *pad;

I would prefer descriptive names instead of part{1,2,3}.

> +    size_t linelen = 0;
> +
> +    fp = fopen(NSCD_CONF_PATH, "r");
> +

I don't see how the blank line here (and elsewhere in the patch)
improves readability.

> +    if (fp == NULL) {
> +        DEBUG(SSSDBG_MINOR_FAILURE, ("Couldn't open NSCD configuration "
> +                    "file [%s]\n", NSCD_CONF_PATH));
> +        return ENOENT;
> +    }
> +
> +    while (getline(&line, &linelen, fp) != -1) {
> +
> +        pad = strchr(line, '#');
> +        if (pad != NULL) {
> +            *pad = '\0';
> +        }
> +
> +        if (line[0] == '\n' || line[0] == '\0') continue;
> +
> +        part1 = line;
> +        while (isspace(*part1) && *part1 != '\0') {
> +            part1++;
> +        }
> +
> +        pad = part1;
> +        while (!isspace(*pad) && *pad != '\0') {
> +            pad++;
> +        }
> +
> +        part2 = pad;
> +        while (isspace(*part2) && *part2 != '\0') {
> +            part2++;
> +        }
> +
> +        *pad = '\0';
> +        pad = part2;
> +        while (!isspace(*pad) && *pad != '\0') {
> +            pad++;
> +        }
> +
> +        part3 = pad;
> +        while (isspace(*part3) && *part3 != '\0') {
> +            part3++;
> +        }
> +
> +        *pad = '\0';
> +        pad = part3;
> +        while (!isspace(*pad) && *pad != '\0') {
> +            pad++;
> +        }
> +        *pad = '\0';
> +
> +        if (!strcmp(part1, "enable-cache") &&
> +            !strcmp(part3, "yes")) {
> +

This works, but I would prefer us to be even more paranoid about the
input, set the parts to NULL before each loop iteration and skipped
lines where either of parts would be NULL here.

> +            occured |= sss_nscd_check_service(part2);
> +        }
> +
> +    };
> +

Can you test here if getline quit because of file IO error vs EOF? The
bahaviour of the caller should be to ignore the error and just print the
old warning, but this function should at least report the error.

> +    fclose(fp);
> +
> +    if (occured != 0) {
> +        return EINVAL;

Maybe EEXIST would be nicer here (or even feel free to include a new
error code). EINVAL feels like something has gone wrong.

> +    }
> +
> +    return EOK;
> +}



More information about the sssd-devel mailing list