[SSSD] [PATCH] Check NSCD configuration file

Jakub Hrozek jhrozek at redhat.com
Tue May 14 14:44:10 UTC 2013


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.

>  WITH_INITSCRIPT
>  if test x$initscript = xsystemd; then
>      WITH_SYSTEMD_UNIT_DIR
> diff --git a/src/conf_macros.m4 b/src/conf_macros.m4
> index 26eb4acccc4895e961ecc50d490a3cdd5716d085..1315fc9da8761a62402692ae6f272236955c2d1c 100644
> --- a/src/conf_macros.m4
> +++ b/src/conf_macros.m4
> @@ -352,6 +352,18 @@ AC_DEFUN([WITH_NSCD],
>      AC_DEFINE_UNQUOTED(HAVE_NSCD, $NSCD_PATH, [flush nscd cache after local domain operations])
>    ])
>  
> +AC_DEFUN([WITH_NSCD_CONF],
> +  [ AC_ARG_WITH([nscd_conf],
> +                [AC_HELP_STRING([--with-nscd-conf=PATH],
> +                                [Path to nscd.conf file [/etc/nscd.conf]])])
> +    NSCD_CONF_PATH="/etc/nscd.conf"
> +    if test x"$with_nscd_conf" != x; then
> +        NSCD_CONF_PATH=$with_nscd_conf
> +        AC_SUBST(NSCD_CONF_PATH)

Why is AC_SUBST used here? I don't see any @NSCD_CONF_PATH at .

> +    fi
> +    AC_DEFINE_UNQUOTED([NSCD_CONF_PATH], ["$NSCD_CONF_PATH"], [NSCD configuration file])
> +  ])
> +
>  AC_DEFUN([WITH_SEMANAGE],
>    [ AC_ARG_WITH([semanage],
>                  [AC_HELP_STRING([--with-semanage],
> diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> index bd22a951d3b48d0ea367e36f628b07e2783d469a..6a69a76fe49a4d6d19561b5d4ae5bed97932b396 100644
> --- a/src/monitor/monitor.c
> +++ b/src/monitor/monitor.c
> @@ -2754,12 +2754,29 @@ int main(int argc, const char *argv[])
>      /* Warn if nscd seems to be running */
>      ret = check_file(NSCD_SOCKET_PATH, -1, -1, -1, CHECK_SOCK, NULL, false);
>      if (ret == EOK) {
> -        sss_log(SSS_LOG_NOTICE,
> -                "nscd socket was detected.  Nscd caching capabilities "
> -                "may conflict with SSSD for users and groups. It is "
> -                "recommended not to run nscd in parallel with SSSD, unless "
> -                "nscd is configured not to cache the passwd, group and "
> -                "netgroup nsswitch maps.");
> +        ret = sss_nscd_parse_conf();
> +
> +        switch (ret) {
> +            case ENOENT:
> +                sss_log(SSS_LOG_NOTICE,
> +                        "NSCD socket was detected, but NSCD configuration "
> +                        "file was not found in the specified path [%s] to "
> +                        "perform  check of its caching settings.",
> +                        NSCD_CONF_PATH);

I would prefer the original sss_log message here instead of an error
message.

> diff --git a/src/util/util.c b/src/util/util.c
> index ba85e0da2a2e03784c5bb9abbbd038ddaeb3455c..8034ee63c632538af74ff7a3238f69cf914a6895 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -688,3 +688,84 @@ void safezero(void *data, size_t size)
>          *p++ = 0;
>      }
>  }
> +
> +/* NSCD config file parse and check */
> +
> +#define NSCD_LINE_BUFF 120
> +
> +enum nscd_service_type {
> +    NSCD_PASSWD = 0,
> +    NSCD_GROUP,
> +    NSCD_NETGROUP,
> +    NSCD_SERVICES
> +};
> +
> +unsigned int nscd_service_flag[4] = {
> +    0x0001, /* passwd   */
> +    0x0010, /* group    */
> +    0x0100, /* netgroup */
> +    0x1000  /* service  */
> +};
> +
> +const char *svc_type_name[] = {
> +    "passwd",
> +    "group",
> +    "netgroup",
> +    "services"
> +};
> +

There could be only one structure instead:

struct sss_nscd_db {
    const char *svc_type_name;
    int nscd_service_flag;
};

The in sss_nscd_check_service you'd have:

struct sss_nscd_db db[] = {
    { passwd, 0x0001 },
    ...
    { NULL, NULL }
};

And the for loop would iterate until db[i].name != NULL. 

This way you don't have to sync three structures. The function could
even be inside the function namespace.

> +unsigned int sss_nscd_check_service(char* svc_name)
> +{
> +    int i;
> +
> +    unsigned int ret = 0;
> +
> +    for (i = NSCD_PASSWD; i <= NSCD_SERVICES; i++) {
> +        if (!strcmp(svc_type_name[i], svc_name)) {
> +
> +            ret |= nscd_service_flag[i];
> +            break;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +errno_t sss_nscd_parse_conf(void)
> +{
> +    FILE *fp;
> +    unsigned int occured = 0;
> +    char line[NSCD_LINE_BUFF];
> +    char part1[40], part2[40], part3[40];
> +
> +    fp = fopen(NSCD_CONF_PATH, "r");
> +
> +    if (fp == NULL) {
> +        DEBUG(SSSDBG_MINOR_FAILURE, ("Couldn't open NSCD configuration "
> +                    "file [%s]\n", NSCD_CONF_PATH));
> +        return ENOENT;
> +    }
> +
> +    while (fgets(line, NSCD_LINE_BUFF, fp) != NULL) {

What about getline instead of gets?

> +
> +        if (line[0] != '#') {
> +

It's more readable to put the main ("success") flow of the program in
the outmost block. So instead of:

    if not_a_comment:
        process()

it is more readable:
    if comment:
        continue

    process()

> +            sscanf(line, "%s %s %s", part1, part2, part3);

Do you know if the parts can only be separated by spaces? I think also
tabs could do.

Also if any part was larger than 40 chars, you'd get a segfault here.

In glibc, the parsing is done in nscd/nscd_conf.c, mostly in
nscd_parse_file() if you'd like to take a look.



More information about the sssd-devel mailing list