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

Stephen Gallagher sgallagh at redhat.com
Tue Sep 8 11:56:53 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I agree with Sumit's review, but I'd rather see his concerns addressed
in this patch, rather than in subsequent patches.

On 09/08/2009 07:04 AM, Sumit Bose wrote:
> 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
>>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel



- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkqmRoEACgkQeiVVYja6o6MeFwCaAvB1ySltQbj7+TORsFXvqZlV
S7YAn3vCV8OVB8l5Mc1x/QEBUDg7Qyzl
=G3Rb
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list