[SSSD] [PATCH] Split database file to one per domain

Sumit Bose sbose at redhat.com
Tue Sep 8 11:04:55 UTC 2009


On Fri, Sep 04, 2009 at 06:02:09PM -0400, Simo Sorce wrote:
> Hello list.
> 
> So far we have been using a single database file for all our caches as
> well as for the local domain.
> 
> Initially I used a single database because I thought we could optimize
> some search queries when it came to enumerations. That didn't prove true
> and in effect we are doing always separate searches for different
> domains (in fact the patch was rather simple to write from this pov as
> no searches needed to be touched at all).
> 
> Using a single database file for all domains is otherwise a
> disadvantage.
> >From a pure "security" point of view each backend has now it's own
> database and cannot "screw up" other domains data.
> Each domain access its backend separately so there is less contention
> when you want to start a transaction.
> The various files are clearly separated, so you can easily just delete
> all the ones named cache-NAME.ldb and not risk deleting the persistent
> data in sssd.ldb
> 
> The patch includes also upgrade routines to automatically backup and
> convert the old file into split files, so switching to the new code is
> painless.
> 
> All the code was converted to handle the new list of database
> structures. One part that felt odd was the sss_tools. They should really
> only operate on the 'local' domain and nothing else. So attaches is also
> a patch that basically reverts most of the changes and just opens the
> 'local' database instead. I have not merged them in a single patch
> because I didn't want to mix format changes with functional changes and
> because apparently we may want to be a bit more radical and eliminate
> also the shadow-utils handling at the same time.
> 
> I have tested the change and the upgrade as well as starting from
> scratch and all seem to work properly.
> 

The patches apply and compile cleanly and are working for me. I have
done some very limited upgrade tests and they worked, too. On the
downside, make check fails, but so far I don't know why, there might be
a problem on my side.

Please find my comments further down in the code, but most of them can
be addressed by later patches. So,

ACK, if you remove the tabs from the patch.

bye,
Sumit
> Simo.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York

> >From 00ed861e0b8c05cbd54b04d7b99ced8d790231f6 Mon Sep 17 00:00:00 2001
> From: Simo Sorce <ssorce at redhat.com>
> Date: Thu, 3 Sep 2009 19:29:41 -0400
> Subject: [PATCH 1/2] Split database in multiple files
> 
> The special persistent local database retains the original name.
> All other backends now have their own cache-NAME.ldb file.
> ---
>  server/Makefile.am                         |    1 +
>  server/confdb/confdb.c                     |   52 ++-
>  server/confdb/confdb.h                     |    2 -
>  server/confdb/confdb_private.h             |    2 +
>  server/db/sysdb.c                          |  770 +++++++++++++++++++++++-----

I think it would make sense to split sysdb.c to sysdb.c, sysdb_init.c
sysdb_upgrade.c or something similar

>  server/db/sysdb.h                          |   18 +-
>  server/db/sysdb_private.h                  |   28 +-
>  server/monitor/monitor.c                   |   17 +-
>  server/providers/data_provider_be.c        |    4 +-
>  server/responder/common/responder.h        |    2 +-
>  server/responder/common/responder_common.c |    4 +-
>  server/responder/nss/nsssrv_cmd.c          |  205 +++++++-
>  server/responder/pam/pam_LOCAL_domain.c    |    8 +-
>  server/responder/pam/pamsrv_cache.c        |    9 +-
>  server/responder/pam/pamsrv_cmd.c          |   29 +-
>  server/tests/sysdb-tests.c                 |    5 +-
>  server/tools/sss_groupadd.c                |   10 +-
>  server/tools/sss_groupdel.c                |   12 +-
>  server/tools/sss_groupmod.c                |   18 +-
>  server/tools/sss_useradd.c                 |   16 +-
>  server/tools/sss_userdel.c                 |   12 +-
>  server/tools/sss_usermod.c                 |   18 +-
>  server/tools/tools_util.c                  |    4 +-
>  server/tools/tools_util.h                  |    3 +-
>  server/util/backup_file.c                  |  119 +++++
>  server/util/util.h                         |    3 +
>  26 files changed, 1137 insertions(+), 234 deletions(-)
>  create mode 100644 server/util/backup_file.c
> 
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 12ec4b1..a56cf01 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -160,6 +160,7 @@ SSSD_UTIL_OBJ = \
>      util/server.c \
>      util/signal.c \
>      util/usertools.c \
> +	util/backup_file.c \

TAB


>      $(SSSD_DEBUG_OBJ)
>  
>  SSSD_RESPONDER_OBJ = \

.................


> --- a/server/db/sysdb.c
> +++ b/server/db/sysdb.c
> @@ -565,6 +565,7 @@ void sysdb_operation_done(struct sysdb_handle *handle)
>  
>  /* =Initialization======================================================== */
>  
> +#if 0
>  static int sysdb_read_var(TALLOC_CTX *mem_ctx,
>                            struct confdb_ctx *cdb,
>                            const char *name,
> @@ -587,123 +588,28 @@ static int sysdb_read_var(TALLOC_CTX *mem_ctx,
>  
>      return EOK;
>  }
> +#endif

Please add a comment why this part is commented out, but not removed.


................

> +static int sysdb_domain_init_internal(TALLOC_CTX *mem_ctx,
> +                                      struct tevent_context *ev,
> +                                      struct sss_domain_info *domain,
> +                                      const char *db_path,
> +                                      bool allow_upgrade,
> +                                      struct sysdb_ctx **_ctx,
> +                                      bool *need_02_upgrade)
> +{
> +    TALLOC_CTX *tmp_ctx = NULL;
> +    struct sysdb_ctx *ctx;
> +    const char *base_ldif;
> +	struct ldb_ldif *ldif;

TAB

> +    struct ldb_message *msg;
> +    struct ldb_message_element *el;
> +    struct ldb_result *res;
> +    struct ldb_dn *verdn;
> +    const char *version = NULL;
> +    int ret;
> +
> +    ctx = talloc_zero(mem_ctx, struct sysdb_ctx);
> +    if (!ctx) {
>          return ENOMEM;
>      }
> +    ctx->ev = ev;
> +    ctx->domain = domain;
>  

.........

> diff --git a/server/util/backup_file.c b/server/util/backup_file.c
> new file mode 100644
> index 0000000..f31d0c7
> --- /dev/null
> +++ b/server/util/backup_file.c
> @@ -0,0 +1,119 @@
> +/*
> + SSSD
> +
> + Backup files
> +
> + Copyright (C) Simo Sorce	2009
> +
> + 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 "util/util.h"
> +#include <fcntl.h>
> +#include <stddef.h>
> +#include <stdlib.h>
> +
> +#define BUFFER_SIZE 65536
> +
> +int backup_file(const char *src, int dbglvl)
> +{
> +    TALLOC_CTX *tmp_ctx = NULL;
> +    char buf[BUFFER_SIZE];
> +    int fds[2] = {-1, -1};

can you use more descriptive names, like src_fd and dest_fd ?

> +    ssize_t num, l, p;

http://freeipa.org/page/Coding_Style#Declaring

> +    char *dst;
> +    int ret, i;
> +
> +    fds[0] = open(src, O_RDONLY);
> +    if (fds[0] < 0) {
> +        ret = errno;
> +        DEBUG(dbglvl, ("Error (%d [%s]) opening source file %s\n",
> +                       ret, strerror(ret), src));
> +        goto done;
> +    }
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (!tmp_ctx) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    /* try a few times to come up with a new backup file, then give up */

why not mkstemp(3) ?

> +    for (i = 0; i < 10; i++) {
> +        if (i == 0) {
> +            dst = talloc_asprintf(tmp_ctx, "%s.bak", src);
> +        } else {
> +            dst = talloc_asprintf(tmp_ctx, "%s.bak%d", src, i);
> +        }
> +        if (!dst) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +
> +        errno = 0;
> +        fds[1] = open(dst, O_CREAT|O_EXCL|O_WRONLY, 0600);
> +        ret = errno;
> +
> +        if (fds[1] > 0) break;
> +
> +        if (ret != EEXIST) {
> +            DEBUG(dbglvl, ("Error (%d [%s]) opening destination file %s\n",
> +                           ret, strerror(ret), dst));
> +            goto done;
> +        }
> +    }
> +    if (ret != 0) {
> +        DEBUG(dbglvl, ("Error (%d [%s]) opening destination file %s\n",
> +                       ret, strerror(ret), dst));
> +        goto done;
> +    }
> +
> +    /* copy file contents */

using mmap might be faster and does not need a buffer.

> +    while (1) {
> +        num = read(fds[0], buf, BUFFER_SIZE);
> +        if (num < 0) {
> +            if (errno == EINTR) continue;
> +            ret = errno;
> +            DEBUG(dbglvl, ("Error (%d [%s]) reading from source %s\n",
> +                           ret, strerror(ret), src));
> +            goto done;
> +        }
> +        if (num == 0) break;
> +
> +        l = num;
> +
> +        while (l > 0) {
> +            p = 0;
> +            errno = 0;
> +            num = write(fds[1], &buf[p], l);
> +            if (num < 0) {
> +                if (errno == EINTR) continue;
> +                ret = errno;
> +                DEBUG(dbglvl, ("Error (%d [%s]) writing to destination %s\n",
> +                               ret, strerror(ret), dst));
> +                goto done;
> +            }
> +            p = l;
> +            l -= num;
> +        }
> +    }
> +
> +    ret = EOK;
> +
> +done:
> +    if (fds[0] != -1) close(fds[0]);
> +    if (fds[1] != -1) close(fds[1]);
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}
> diff --git a/server/util/util.h b/server/util/util.h
> index f289f9c..93e5dc9 100644
> --- a/server/util/util.h
> +++ b/server/util/util.h
> @@ -157,4 +157,7 @@ int sss_parse_name(TALLOC_CTX *memctx,
>                     struct sss_names_ctx *snctx,
>                     const char *orig, char **domain, char **name);
>  
> +/* from backup-file.c */
> +int backup_file(const char *src, int dbglvl);
> +
>  #endif /* __SSSD_UTIL_H__ */
> -- 
> 1.6.2.5
> 



More information about the sssd-devel mailing list