[SSSD] [PATCH] Check NSCD configuration file

Lukas Slebodnik lslebodn at redhat.com
Fri May 17 11:50:52 UTC 2013


On (17/05/13 10:29), Jakub Hrozek wrote:
>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);
>> +        }
>> +
>> +    };
You forgot to release memory after "while"
free(line);

See EXAMPLE section in man 3 getline.

>
>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;
>> +}
>_______________________________________________
>sssd-devel mailing list
>sssd-devel at lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list