[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