Re: [SSSD] [PATCH] sss_override: support import and export

[Lukas Slebodnik]

On (16/08/15 17:59), Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/2737
>
> Hi, it should work... :-) However, I wanted to make import as transaction so
> no changes are made if some error occurs, but I had some troubles with it so
> I gave up eventually.
>
> Of course, it would be quite difficult to make it safe across domains thus my
> intention was only to ensure that either all changes are done in a domain or
> none. But still leaving the possibility that changes are commited in one
> domain but cancelled in another.
>
> I tried to start sysdb transaction on all used domains and then commit/cancel
> it, writing some neat mechanism for it. However, I occasionally run into a
> problem when data provider hangs when trying to safe a user. It looked like
> some sort of race condition.
>
> Unfortunately I managed to delete the code so I can't show it to you, I think
> it would be a nice feature so if anyone familiar with ldb want to step in,
> he's welcome.

I briefly look at the patches and here are few comments
> From 24655f677acf9f64201a611ababdcfeff4317037 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Sat, 15 Aug 2015 16:42:27 +0200
> Subject: [PATCH 2/2] sss_override: add import and export
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2737
>
> Makefile.am                    |   2 +
> src/tools/common/sss_colondb.c | 298 ++++++++++++++
> src/tools/common/sss_colondb.h |  71 ++++
> src/tools/sss_override.c       | 866 +++++++++++++++++++++++++++++++++++------
> 4 files changed, 1128 insertions(+), 109 deletions(-)
> create mode 100644 src/tools/common/sss_colondb.c
> create mode 100644 src/tools/common/sss_colondb.h
>
> diff --git a/Makefile.am b/Makefile.am
> index ed107fd5dc76b768176a3d7236b0bf1c75f212bf..f8c7c29df08c2ba9e74508c0f84ac57f6e6bac26 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -651,6 +651,7 @@ dist_noinst_HEADERS = \
>     src/lib/sifp/sss_sifp_private.h \
>     src/tests/cmocka/test_utils.h \
>     src/tools/common/sss_tools.h \
> src/tools/common/sss_colondb.h \
> $(NULL)
>
> @@ -1331,6 +1332,7 @@ sss_signal_LDADD = \
>
> sss_override_SOURCES = \
>     src/tools/sss_override.c \
> src/tools/common/sss_colondb.c \
> $(SSSD_TOOLS_OBJ) \
> $(NULL)
>
> sss_override_LDADD = \
> diff --git a/src/tools/common/sss_colondb.c b/src/tools/common/sss_colondb.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..be9396832e93fa3b646255dae214a34fe4d6c29c
> --- /dev/null
> +++ b/src/tools/common/sss_colondb.c
> @@ -0,0 +1,298 @@
> +/*
> Authors:
>    Pavel Březina <[e-mail]>
> Copyright (C) 2015 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 <stdlib.h>
>
> +#include "util/util.h"
> +#include "util/strtonum.h"
> +#include "tools/common/sss_colondb.h"
>
> +#define IS_STD_FILE(db) ((db)->file == stdin || (db)->file == stdout)
> +#define IS_LINE_END(str) ((str) == NULL || *(str) == '\0' || *(str) == '\n')

^^^^^^^^^^^
          This macro is unused.
          Side note: if there is expectation occurrence '\n' should be higher
          that occurrence of '\0' then you should change order in condition.

//snip
> +errno_t sss_colondb_writeline(struct sss_colondb *db,
>                          struct sss_colondb_write_field *table)
>
> +{
> TALLOC_CTX *tmp_ctx;
> char *line = NULL;
> errno_t ret;
> int i;
> if (db->mode != SSS_COLONDB_WRITE) {
>    return ERR_INTERNAL;
> }
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n");
>    return ENOMEM;
> }
> for (i = 0; table[i].type != SSS_COLONDB_SENTINEL; i++) {
>    switch (table[i].type) {
>    case SSS_COLONDB_UINT32:
>        if (table[i].data.uint32 == 0) {
>            line = talloc_asprintf_append(line, ":");
>        } else {
>            line = talloc_asprintf_append(line, ":%u", table[i].data.uint32);

uint32_t has different fotmat specifier
                                       @see man inttypes.h
>        break;
>    case SSS_COLONDB_STRING:
>        if (table[i].data.str == NULL) {
>            line = talloc_asprintf_append(line, ":");
>        } else {
>            line = talloc_asprintf_append(line, ":%s", table[i].data.str);

Maybe I'm wrong but the first entry in /etc/passwd is string
                 and does not start with ":". I'm not sure about Dell VAS
                 format.
                 I cannot see any special case for index 0
>        }
>        break;
>    case SSS_COLONDB_SENTINEL:
>        DEBUG(SSSDBG_CRIT_FAILURE, "Trying to process sentinel?!\n");
>        ret = ERR_INTERNAL;
>        goto done;
>    }
>    if (line == NULL) {
>        ret = ENOMEM;
>        goto done;
>    }
> }
> /* Remove starting : */
> line++;
> fprintf(db->file, "%s\n", line);
> fflush(db->file);
> ret = EOK;
>
> +done:
> talloc_free(tmp_ctx);
> return ret;
>
> +}

LS