[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