[SSSD] [PATCH] DB: Switch to new libini_config API

Jakub Hrozek jhrozek at redhat.com
Fri Apr 19 19:00:12 UTC 2013


On Fri, Apr 19, 2013 at 04:05:49PM +0200, Ondrej Kos wrote:
> This was really weird, it worked correctly in my environment, even
> if i rebased on the current master, but started segfaulting when i
> backed few patches. The code was wrong, but it wasn't failing.
> 
> >
> >We also did a short review in person. tl;dr -- do not use void* but make
> >the structure opaque and always pass the pointer to the structure
> >regardless of its contents.
> 
> My first idea was for the functions to accept just the necessary
> variables, but this will be better, the code is more readable now
> and the maintenance will be easier, thanks for advice!
> 
> New patch is attached
> 
> Ondra

Thank you, some more comments are inline:

>  int confdb_init_db(const char *config_file, struct confdb_ctx *cdb)
>  {
> +    TALLOC_CTX *tmp_ctx;
>      int ret;
> -    int fd = -1;
> -    struct collection_item *sssd_config = NULL;
> -    struct collection_item *error_list = NULL;
> -    struct collection_item *item = NULL;
> +    int version;
> +    char timestr[21];
> +    char *lasttimestr;
>      const char *config_ldif;
> -    struct ldb_ldif *ldif;
> -    TALLOC_CTX *tmp_ctx;
> -    char *lasttimestr, timestr[21];
>      const char *vals[2] = { timestr, NULL };
> -    struct stat cstat;
> -    int version;
> +    struct ldb_ldif *ldif;
> +    struct sss_ini_initdata *init_data;
> +
>  

I know that the transaction logic in this function predates this patch,
but I think we should fix it when we are changing the code anyway. Can
you take a look at how we use the "in_transaction" booleans elsewhere
for sysdb transaction and put the same logic here?

Then you would always be able to just goto done and have the cleanup
done on one place.

>      tmp_ctx = talloc_new(cdb);
> -    if (tmp_ctx == NULL) return ENOMEM;
> +    if (tmp_ctx == NULL) {
> +        DEBUG(SSSDBG_FATAL_FAILURE, ("Out of memory.\n"));
> +        return ENOMEM;
> +    }
>  
> -    ret = check_and_open_readonly(config_file, &fd, 0, 0, (S_IRUSR|S_IWUSR),
> -                                  CHECK_REG);
> +    init_data = sss_ini_initdata_init(tmp_ctx);
> +    if (!init_data) {
> +        DEBUG(SSSDBG_FATAL_FAILURE, ("Out of memory.\n"));

You're leaking tmp_ctx here and on several other places. The advice
above would fix this problem.

> +        return ENOMEM;
> +    }
> +

[snip]

> -    ret = fstat(fd, &cstat);
> -    if (ret != 0) {
> -        DEBUG(0, ("Unable to stat config file [%s]! (%d [%s])\n",
> -                  config_file, errno, strerror(errno)));
> -        close(fd);
> +    ret = sss_ini_get_stat(init_data);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_FATAL_FAILURE,
> +                ("No stat data though stat collection was requested.\n"));

I don't understand this error message, can you reword it?

> +        sss_ini_close_file(init_data);
>          talloc_zfree(tmp_ctx);
>          return errno;
>      }
> -    ret = snprintf(timestr, 21, "%llu", (long long unsigned)cstat.st_mtime);
> +
> +    errno = 0;
> +
> +    ret = sss_ini_get_mtime(timestr, init_data);

This function really should have a size_t parameter, an API cannot
make any assumptions about the size of an input buffer, sorry.

>      if (ret <= 0 || ret >= 21) {

you can use sizeof(timestr) here in the odd case timestr actually
changed.

[snip]

> -    /* Purge existing database */
> -    ret = confdb_purge(cdb);
> -    if (ret != EOK) {
> -        DEBUG(0, ("Could not purge existing configuration\n"));
> -        close(fd);
> +    } else {
> +        DEBUG(SSSDBG_FATAL_FAILURE, ("Failed to get lastUpdate attribute.\n"));
>          goto done;

goto done is currently wrong here, it would attempt to cancel a
transaction that hasn't started yet.

[snip]

> +    version = sss_ini_get_int_config_value(init_data, 1, -1, &ret);
>      if (ret != EOK) {
> -        DEBUG(0, ("Config file version could not be determined\n"));
> +        DEBUG(SSSDBG_FATAL_FAILURE,
> +                ("Config file version could not be determined\n"));

All previous error handlers called sss_ini_close_file() and
sss_ini_config_destroy() is being called from now on. Is that correct?

> diff --git a/src/external/libini_config.m4 b/src/external/libini_config.m4
> index f41f31917d1cf12dda49ec2e1f2411e39dbcc759..fc2c2a4456e05b7fb1354eb96fe124192f411d85 100644
> --- a/src/external/libini_config.m4
> +++ b/src/external/libini_config.m4
> @@ -1,10 +1,22 @@
> +PKG_CHECK_MODULES(INI_CONFIG, [
> +    ini_config >= 1.0.0], [
> +
> +        HAVE_LIBINI_CONFIG_V1=1

This assignment seems redundant to me. AC_DEFINE_UNQUOTED accepts an
identifier as the first parameter.

> +        AC_DEFINE_UNQUOTED(HAVE_LIBINI_CONFIG_V1, 1, [libini_config version greater than 1.0.0])
> +    ], [
> +        AC_MSG_WARN([libini_config-devel >= 1.0.0 not available, trying older version])
> +        PKG_CHECK_MODULES(INI_CONFIG, [
> +            ini_config >= 0.6.1], [
> +
> +                HAVE_LIBINI_CONFIG_V0=1

Not needed either.

> +                AC_DEFINE_UNQUOTED(HAVE_LIBINI_CONFIG_V0, 1, [libini_config version lesser than 1.0.0])
> +            ], [
> +                AC_MSG_ERROR([Please install libini_config-devel])
> +            ]
> +        )
> +    ]
> +)
> --- /dev/null
> +++ b/src/util/sss_ini.c
> @@ -0,0 +1,466 @@
> +/*
> +    SSSD
> +
> +    sss_ini.c
> +
> +    Authors:
> +        Ondrej Kos <okos at redhat.com>
> +
> +    Copyright (C) 2013 Red Hat
> +
> +    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/>.
> +*/
> +
> +#include <stdio.h>
> +#include <errno.h>
> +#include <talloc.h>
> +
> +#include "config.h"
> +#include "util/util.h"
> +#include "util/sss_ini.h"
> +#include "confdb/confdb_setup.h"
> +#include "confdb/confdb_private.h"
> +
> +#ifdef HAVE_LIBINI_CONFIG_V1
> +#include "ini_configobj.h"
> +#endif
> +#include "ini_config.h"
> +
> +#ifdef HAVE_LIBINI_CONFIG_V0
> +#include "collection.h"
> +#include "collection_tools.h"
> +#endif

We should be paranoid here and have an #else that would contain an
#error directive. It is OK to add this just once, I guess, the
compiler would error out.

> +/* Initialize data structure */
> +
> +struct sss_ini_initdata* sss_ini_initdata_init(TALLOC_CTX *tmp_ctx)

Memory context that allocates output directly should not be named
tmp_ctx but mem_ctx probably.

> +{
> +    return talloc_zero(tmp_ctx, struct sss_ini_initdata);
> +}
> +
> +
> +
> +/* Close file descriptor */
> +
> +void sss_ini_close_file(struct sss_ini_initdata *init_data)
> +{

It is a custom with any close/free functions in most projects to
gracefully handle if the object being closed is not initialized.

So please add here:
    if (init_data == NULL || init_data->return == NULL) {
        return;
    }

The same needs to be done for sss_ini_free_ini_config and
sss_ini_config_destroy.

> +#ifdef HAVE_LIBINI_CONFIG_V1
> +    ini_config_file_destroy(init_data->file);
> +#elif HAVE_LIBINI_CONFIG_V0
> +    close(init_data->file);
> +#endif
> +}
> +
> +
> +
> +/* Open configuration file */
> +
> +int sss_ini_config_file_open(const char *config_file,
> +                             struct sss_ini_initdata *init_data)
> +{
> +#ifdef HAVE_LIBINI_CONFIG_V1
> +    return ini_config_file_open(config_file,
> +                                INI_META_STATS,
> +                                &init_data->file);
> +#elif HAVE_LIBINI_CONFIG_V0
> +    return check_and_open_readonly(config_file, &init_data->file, 0, 0,
> +                                   (S_IRUSR|S_IWUSR), CHECK_REG);
> +#endif
> +}
> +
> +
> +
> +/* Check configuration file permissions */
> +
> +int sss_ini_config_access_check(struct sss_ini_initdata *init_data)
> +{
> +#ifdef HAVE_LIBINI_CONFIG_V1
> +    return ini_config_access_check(init_data->file,
> +                                   INI_ACCESS_CHECK_MODE |
> +                                   INI_ACCESS_CHECK_UID |
> +                                   INI_ACCESS_CHECK_GID,
> +                                   0, /* owned by root */
> +                                   0, /* owned by root */
> +                                   (S_IRUSR|S_IWUSR), /* rw------- */
> +                                   0); /* check all there parts */
> +#elif HAVE_LIBINI_CONFIG_V0
> +    return EOK;
> +#endif
> +}
> +
> +
> +
> +/* Get cstat */
> +
> +int sss_ini_get_stat(struct sss_ini_initdata *init_data)
> +{
> +#ifdef HAVE_LIBINI_CONFIG_V1
> +    init_data->cstat = ini_config_get_stat(file_desc);
> +
> +    if (!init_data->cstat) return EIO;
> +
> +    return EOK;
> +#elif HAVE_LIBINI_CONFIG_V0
> +
> +    return fstat(init_data->file, &init_data->cstat);
> +#endif
> +}
> +
> +
> +
> +/* Get mtime */
> +
> +int sss_ini_get_mtime(char *timestr, struct sss_ini_initdata *init_data)
> +{
> +#ifdef HAVE_LIBINI_CONFIG_V1
> +    return snprintf(timestr, 21, "%llu",
                              ^^^^^^^
            This function should either have a big fat comment saying that
            timestr is at least 21 bytes large, or (better) accept also
            a size_t parameter that specifies the actual size.
> +                    (long long unsigned)init_data->cstat->st_mtime);
> +#elif HAVE_LIBINI_CONFIG_V0
> +    return snprintf(timestr, 21, "%llu",
> +                    (long long unsigned)init_data->cstat.st_mtime);
> +#endif
> +}
> +
> +
> +
> +/* Load configuration */
> +
> +int sss_ini_get_config(const char *config_file,
> +                       struct sss_ini_initdata *init_data)
> +{
> +    int ret;
> +#ifdef HAVE_LIBINI_CONFIG_V1
> +
> +    /* Create config object */
> +    ret = ini_config_create(&(init_data->sssd_config));
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_FATAL_FAILURE,
> +                ("Failed to create config object. Error %d.\n", ret));
> +        return ret;
> +    }
> +
> +    /* Parse file */
> +    ret = ini_config_parse(init_data->file,
> +                           INI_STOP_ON_ANY,
> +                           0, /* <- check that you really want this */

What does this comment mean?

> +                           0,
> +                           init_data->sssd_config);
> +
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_FATAL_FAILURE,
> +                ("Failed to parse configuration. Error %d.\n", ret));
> +
> +        if (ini_config_error_count(init_data->sssd_config)) {
> +            DEBUG(SSSDBG_FATAL_FAILURE,
> +                    ("Errors detected while parsing: %s\n",
> +                     ini_config_get_filename(init_data->file));
> +
> +            ini_config_get_errors(init_data->sssd_config,
> +                                  &init_data->error_list);
> +            ini_config_print_errors(stderr, init_data->error_list));

You can't unconditionally use stderr here, if sssd was running as a
deamon, stderr would be closed. Use debug_file instead.

> +            ini_config_free_errors(init_data->error_list);
> +        }
> +        ini_config_destroy(init_data->sssd_config);
> +        return ret;
> +    }
> +
> +    return ret;
> +
> +#elif HAVE_LIBINI_CONFIG_V0
> +
> +    /* Read the configuration into a collection */
> +    ret = config_from_fd("sssd",
> +                         init_data->file,
> +                         config_file,
> +                         &init_data->sssd_config,
> +                         INI_STOP_ON_ANY,
> +                         &init_data->error_list);
> +    close(init_data->file);
> +
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_FATAL_FAILURE,
> +                ("Parse error reading configuration file [%s]\n",
> +                 config_file));
> +
> +        print_file_parsing_errors(stderr, init_data->error_list);
> +
> +        free_ini_config_errors(init_data->error_list);
> +        free_ini_config(init_data->sssd_config);
> +
> +        return ret;
> +    }
> +
> +    return ret;

You can safely return EOK here, this path is reachable on success only right?
> +
> +#endif
> +}

[snip]

> +/* Free ini config (v0) */
> +
> +void sss_ini_free_ini_config(struct sss_ini_initdata *init_data)
> +{
> +#ifdef HAVE_LIBINI_CONFIG_V1
> +    return;
> +#elif HAVE_LIBINI_CONFIG_V0
> +    free_ini_config(init_data->sssd_config);
> +#endif
> +}
> +
> +
> +
> +/* Destroy ini config (v1) */
> +
> +void sss_ini_config_destroy(struct sss_ini_initdata *init_data)
> +{
> +#ifdef HAVE_LIBINI_CONFIG_V1
> +    ini_config_destroy(init_data->sssd_config);
> +#elif HAVE_LIBINI_CONFIG_V0
> +    return;
> +#endif
> +}
> +

I don't get why these functions are separate, sounds like you should
merge them to me?
> +
> +
> +/* Create LDIF */
> +
> +int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx,
> +                           struct sss_ini_initdata *init_data,
> +                           const char **config_ldif)

There are some old-style DEBUG messages left in this function.

[snip]

> +#ifndef __SSS_INI_H__
> +#define __SSS_INI_H__
> +
> +/* Structure declarations */
> +
> +/* INI data structure */
> +struct sss_ini_initdata;
> +
> +
> +/* Function declarations */
> +
> +/* Initialize data structure */
> +struct sss_ini_initdata* sss_ini_initdata_init(TALLOC_CTX *tmp_ctx);
> +
> +/* Close file descriptor */
> +void sss_ini_close_file(struct sss_ini_initdata *init_data);
> +
> +/* Open config file */
> +int sss_ini_config_file_open(const char *config_file,
> +                             struct sss_ini_initdata *init_data);
> +
> +/* Check file permissions */
> +int sss_ini_config_access_check(struct sss_ini_initdata *init_data);
> +
> +/* Cstat */
> +int sss_ini_get_stat(struct sss_ini_initdata *init_data);
> +

This new wrapper interface looks much better to me, but in general when
designing such quasi-object interfaces that operate on a structure, the
structure should always be the first (with the exception of talloc context)
parameter and especially any output parameters should always come last.

sss_confdb_create_ldif() got it right.

> +/* Get mtime */
> +int sss_ini_get_mtime(char *timestr, struct sss_ini_initdata *init_data);

Functionally the code seems to be working well.



More information about the sssd-devel mailing list