[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