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(a)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.