[PATCH] sss_override: support import and export

[Pavel Březina]

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.

[Jakub Hrozek]

On Sun, Aug 16, 2015 at 05:59:22PM +0200, 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. 
Thanks a lot for the patches. I didn't do any review yet, just scrolled
through them.

My first reaction was that the second patch should be split up and at
least the colondb should be unit tested.

btw did you get inspired by some existing code, like libuser or
shadow-utils with the colondb?

>  
>  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. 
>  From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  Date: Sat, 15 Aug 2015 13:53:25 +0200
>  Subject: [PATCH 1/2] sss_override: print input name if unable to parse it
>  
>  ---
>   src/tools/sss_override.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>  
>  diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c
>  index 5e901e2e31de64dacb171337defc03d428f8ed57..e84a7b922dfcf179f8010dc4cced0eafd89a2c76
> 100644
>  --- a/src/tools/sss_override.c
>  +++ b/src/tools/sss_override.c
>  @@ -74,7 +74,7 @@ static int parse_cmdline(struct sss_cmdline *cmdline,
>       ret = sss_tool_parse_name(tool_ctx, tool_ctx, input_name,
>                                 &orig_name, &domain);
>       if (ret != EOK) {
>  -        fprintf(stderr, _("Unable to parse name.\n"));
>  +        fprintf(stderr, _("Unable to parse name %s.\n"), input_name);
>           return ret;
>       }
>   
>  -- 
>  1.9.3
>   
>  From 24655f677acf9f64201a611ababdcfeff4317037 Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  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 <pbrezina(a)redhat.com&gt;
>  +
>  +    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')
>  +
>  +static char *read_field_as_string(char *line,
>  +                                  const char **_value)
>  +{
>  +    char *rest;
>  +    char *value;
>  +
>  +    if (line == NULL || *line == '\0' || *line == '\n') {
>  +        /* There is nothing else to read. */
>  +        rest = NULL;
>  +        value = NULL;
>  +        goto done;
>  +    }
>  +
>  +    if (*line == ':') {
>  +        /* Special case for empty value. */
>  +        *line = '\0';
>  +        rest = line + 1;
>  +        value = NULL;
>  +        goto done;
>  +    }
>  +
>  +    /* Value starts at current position. */
>  +    value = line;
>  +
>  +    /* Find next field delimiter. */
>  +    rest = strchr(line, ':');
>  +    if (rest == NULL) {
>  +        /* There is no more field. Remove \n from the end. */
>  +        rest = strchr(line, '\n');
>  +        if (rest != NULL) {
>  +            *rest = '\0';
>  +            rest = NULL;
>  +        }
>  +        goto done;
>  +    }
>  +
>  +    /* Remove it and step one character further. */
>  +    *rest = '\0';
>  +    rest++;
>  +
>  +done:
>  +    *_value = value;
>  +
>  +    return rest;
>  +}
>  +
>  +static char *read_field_as_uint32(char *line,
>  +                                  uint32_t *_value)
>  +{
>  +    const char *str;
>  +    char *rest;
>  +    errno_t ret;
>  +
>  +    rest = read_field_as_string(line, &str);
>  +    if (rest == NULL || str == NULL) {
>  +        *_value = 0;
>  +        return rest;
>  +    }
>  +
>  +    *_value = strtouint32(str, NULL, 10);
>  +    if (errno != 0) {
>  +        ret = errno;
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse number [%d]: %s\n",
>  +              ret, sss_strerror(ret));
>  +
>  +        *_value = 0;
>  +    }
>  +
>  +    return rest;
>  +}
>  +
>  +struct sss_colondb {
>  +    FILE *file;
>  +    enum sss_colondb_mode mode;
>  +};
>  +
>  +errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
>  +                             struct sss_colondb *db,
>  +                             struct sss_colondb_read_field *table)
>  +{
>  +    int readchars;
>  +    size_t linelen = 0;
>  +    char *line = NULL;
>  +    char *rest;
>  +    errno_t ret;
>  +    int i;
>  +
>  +    if (db->mode != SSS_COLONDB_READ) {
>  +        return ERR_INTERNAL;
>  +    }
>  +
>  +    readchars = getline(&line, &linelen, db->file);
>  +    if (readchars == -1) {
>  +        /* Nothing was read. */
>  +        if (errno != 0) {
>  +            ret = errno;
>  +            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]: %s",
>  +                  ret, sss_strerror(ret));
>  +            return ret;
>  +        }
>  +
>  +        return EOF;
>  +    }
>  +
>  +    /* Copy line to mem_ctx. */
>  +    rest = talloc_strdup(mem_ctx, line);
>  +    if (rest == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup() failed\n");
>  +        return ENOMEM;
>  +    }
>  +    free(line);
>  +    line = rest;
>  +
>  +    for (i = 0; table[i].type != SSS_COLONDB_SENTINEL; i++) {
>  +        switch (table[i].type) {
>  +        case SSS_COLONDB_UINT32:
>  +            rest = read_field_as_uint32(rest, table[i].data.uint32);
>  +            break;
>  +        case SSS_COLONDB_STRING:
>  +            rest = read_field_as_string(rest, table[i].data.str);
>  +            break;
>  +        case SSS_COLONDB_SENTINEL:
>  +            DEBUG(SSSDBG_CRIT_FAILURE, "Trying to process sentinel?!\n");
>  +            ret = ERR_INTERNAL;
>  +            goto done;
>  +        }
>  +
>  +        if (rest == NULL && table[i + 1].type != SSS_COLONDB_SENTINEL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE,
>  +                  "Line contains less values than expected!\n");
>  +            ret = EINVAL;
>  +            goto done;
>  +        } else if (rest != NULL && table[i + 1].type == SSS_COLONDB_SENTINEL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE,
>  +                  "Line contains more values than expected!\n");
>  +            ret = EINVAL;
>  +            goto done;
>  +        }
>  +    }
>  +
>  +    ret = EOK;
>  +
>  +done:
>  +    if (ret != EOK) {
>  +        talloc_free(line);
>  +    }
>  +
>  +    return ret;
>  +}
>  +
>  +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);
>  +            }
>  +            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);
>  +            }
>  +            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;
>  +}
>  +
>  +static int sss_colondb_close(void *pvt)
>  +{
>  +    struct sss_colondb *db = talloc_get_type(pvt, struct sss_colondb);
>  +
>  +    if (db->file == NULL || IS_STD_FILE(db)) {
>  +        return 0;
>  +    }
>  +
>  +    fclose(db->file);
>  +    db->file = NULL;
>  +
>  +    return 0;
>  +}
>  +
>  +struct sss_colondb *sss_colondb_open(TALLOC_CTX *mem_ctx,
>  +                                     enum sss_colondb_mode mode,
>  +                                     const char *filename)
>  +{
>  +    FILE *fp;
>  +    const char *fmode;
>  +    struct sss_colondb *db;
>  +
>  +    switch (mode) {
>  +    case SSS_COLONDB_READ:
>  +        fmode = "r";
>  +        if (filename == NULL) {
>  +            fp = stdin;
>  +        }
>  +        break;
>  +    case SSS_COLONDB_WRITE:
>  +        fmode = "w";
>  +        if (filename == NULL) {
>  +            fp = stdout;
>  +        }
>  +        break;
>  +    }
>  +
>  +    if (filename != NULL) {
>  +        errno = 0;
>  +        fp = fopen(filename, fmode);
>  +        if (fp == NULL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s\n", filename);
>  +            return NULL;
>  +        }
>  +    }
>  +
>  +    db = talloc_zero(mem_ctx, struct sss_colondb);
>  +    if (db == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero() failed\n");
>  +        return NULL;
>  +    }
>  +
>  +    db->file = fp;
>  +    db->mode = mode;
>  +
>  +    talloc_set_destructor((TALLOC_CTX *)db, sss_colondb_close);
>  +
>  +    return db;
>  +}
>  diff --git a/src/tools/common/sss_colondb.h b/src/tools/common/sss_colondb.h
>  new file mode 100644
>  index 0000000000000000000000000000000000000000..13e9c010bb6a233a5f939b453f80b29622fff841
>  --- /dev/null
>  +++ b/src/tools/common/sss_colondb.h
>  @@ -0,0 +1,71 @@
>  +/*
>  +    Authors:
>  +        Pavel Březina <pbrezina(a)redhat.com&gt;
>  +
>  +    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/>;.
>  +*/
>  +
>  +#ifndef _SSS_COLONDB_H_
>  +#define _SSS_COLONDB_H_
>  +
>  +#include <stdlib.h>
>  +#include <talloc.h>
>  +
>  +struct sss_colondb;
>  +
>  +enum sss_colondb_mode {
>  +    SSS_COLONDB_READ,
>  +    SSS_COLONDB_WRITE
>  +};
>  +
>  +enum sss_colondb_type {
>  +    SSS_COLONDB_UINT32,
>  +    SSS_COLONDB_STRING,
>  +    SSS_COLONDB_SENTINEL
>  +};
>  +
>  +union sss_colondb_write_data {
>  +    uint32_t uint32;
>  +    const char *str;
>  +};
>  +
>  +union sss_colondb_read_data {
>  +    uint32_t *uint32;
>  +    const char **str;
>  +};
>  +
>  +struct sss_colondb_write_field {
>  +    enum sss_colondb_type type;
>  +    union sss_colondb_write_data data;
>  +};
>  +
>  +struct sss_colondb_read_field {
>  +    enum sss_colondb_type type;
>  +    union sss_colondb_read_data data;
>  +};
>  +
>  +struct sss_colondb *sss_colondb_open(TALLOC_CTX *mem_ctx,
>  +                                     enum sss_colondb_mode mode,
>  +                                     const char *filename);
>  +
>  +errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
>  +                             struct sss_colondb *db,
>  +                             struct sss_colondb_read_field *table);
>  +
>  +errno_t sss_colondb_writeline(struct sss_colondb *db,
>  +                              struct sss_colondb_write_field *table);
>  +
>  +#endif /* _SSS_COLONDB_H_ */
>  diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c
>  index e84a7b922dfcf179f8010dc4cced0eafd89a2c76..40d0aa233be0b3c8f64c7102ca8cb3b250ca151b
> 100644
>  --- a/src/tools/sss_override.c
>  +++ b/src/tools/sss_override.c
>  @@ -23,8 +23,10 @@
>   #include "util/util.h"
>   #include "db/sysdb.h"
>   #include "tools/common/sss_tools.h"
>  +#include "tools/common/sss_colondb.h"
>   
>   #define LOCALVIEW SYSDB_LOCAL_VIEW_NAME
>  +#define ORIGNAME "originalName"
>   
>   struct override_user {
>       const char *input_name;
>  @@ -135,6 +137,40 @@ static int parse_cmdline_group_del(struct sss_cmdline *cmdline,
>                            &group->orig_name, &group->domain);
>   }
>   
>  +static int parse_cmdline_import(struct sss_cmdline *cmdline,
>  +                                struct sss_tool_ctx *tool_ctx,
>  +                                const char **_file)
>  +{
>  +    int ret;
>  +
>  +    ret = sss_tool_popt_ex(cmdline, NULL, SSS_TOOL_OPT_OPTIONAL,
>  +                           NULL, NULL, "FILE", "File to import the data
> from.",
>  +                           _file);
>  +    if (ret != EXIT_SUCCESS) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command arguments\n");
>  +        return ret;
>  +    }
>  +
>  +    return EXIT_SUCCESS;
>  +}
>  +
>  +static int parse_cmdline_export(struct sss_cmdline *cmdline,
>  +                                struct sss_tool_ctx *tool_ctx,
>  +                                const char **_file)
>  +{
>  +    int ret;
>  +
>  +    ret = sss_tool_popt_ex(cmdline, NULL, SSS_TOOL_OPT_OPTIONAL,
>  +                           NULL, NULL, "FILE", "File to export the data
> to.",
>  +                           _file);
>  +    if (ret != EXIT_SUCCESS) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command arguments\n");
>  +        return ret;
>  +    }
>  +
>  +    return EXIT_SUCCESS;
>  +}
>  +
>   static errno_t prepare_view(struct sss_domain_info *domain)
>   {
>       char *viewname = NULL;
>  @@ -171,6 +207,22 @@ done:
>       return ret;
>   }
>   
>  +errno_t prepare_view_msg(struct sss_domain_info *domain)
>  +{
>  +    errno_t ret;
>  +
>  +    ret = prepare_view(domain);
>  +    if (ret == EEXIST) {
>  +        fprintf(stderr, _("Other than " LOCALVIEW " view already exist
> "
>  +                "in domain %s.\n"), domain->name);
>  +    } else if (ret != EOK) {
>  +        fprintf(stderr, _("Unable to prepare " LOCALVIEW
>  +                " view in domain %s.\n"), domain->name);
>  +    }
>  +
>  +    return ret;
>  +}
>  +
>   static char *build_anchor(TALLOC_CTX *mem_ctx, const char *obj_dn)
>   {
>       char *anchor;
>  @@ -272,17 +324,15 @@ static struct sysdb_attrs *build_group_attrs(TALLOC_CTX *mem_ctx,
>       return build_attrs(mem_ctx, group->name, 0, group->gid, 0, NULL, NULL);
>   }
>   
>  -static const char *get_object_dn_and_domain(TALLOC_CTX *mem_ctx,
>  -                                         enum sysdb_member_type type,
>  -                                         const char *name,
>  -                                         struct sss_domain_info *domain,
>  -                                         struct sss_domain_info *domains,
>  -                                         struct sss_domain_info **_new_domain)
>  +static struct sss_domain_info *
>  +get_object_domain(enum sysdb_member_type type,
>  +                  const char *name,
>  +                  struct sss_domain_info *domain,
>  +                  struct sss_domain_info *domains)
>   {
>       TALLOC_CTX *tmp_ctx;
>       struct sss_domain_info *dom;
>       struct ldb_result *res;
>  -    const char *dn;
>       const char *strtype;
>       bool check_next;
>       errno_t ret;
>  @@ -372,18 +422,6 @@ static const char *get_object_dn_and_domain(TALLOC_CTX *mem_ctx,
>       DEBUG(SSSDBG_TRACE_FUNC, "Domain of %s %s is %s\n",
>             strtype, name, dom->name);
>   
>  -    dn = ldb_dn_get_linearized(res->msgs[0]->dn);
>  -    if (dn == NULL) {
>  -        DEBUG(SSSDBG_CRIT_FAILURE, "ldb_dn_get_linearized() failed.\n");
>  -        ret = ENOMEM;
>  -        goto done;
>  -    }
>  -
>  -    talloc_steal(mem_ctx, dn);
>  -    *_new_domain = dom;
>  -
>  -    ret = EOK;
>  -
>   done:
>       talloc_free(tmp_ctx);
>   
>  @@ -391,35 +429,94 @@ done:
>           return NULL;
>       }
>   
>  -    return dn;
>  +    return dom;
>   }
>   
>  -static const char * get_user_dn_and_domain(TALLOC_CTX *mem_ctx,
>  -                                           struct sss_domain_info *domains,
>  -                                           struct override_user *user)
>  +static errno_t get_user_domain_msg(struct sss_tool_ctx *tool_ctx,
>  +                                   struct override_user *user)
>   {
>  -    return get_object_dn_and_domain(mem_ctx, SYSDB_MEMBER_USER,
>  -                         user->orig_name, user->domain, domains,
>  -                         &user->domain);
>  +    struct sss_domain_info *newdom;
>  +    const char *domname;
>  +
>  +    newdom = get_object_domain(SYSDB_MEMBER_USER, user->orig_name,
>  +                               user->domain, tool_ctx->domains);
>  +    if (newdom == NULL) {
>  +        domname = user->domain == NULL ? "[unknown]" :
> user->domain->name;
>  +        fprintf(stderr, _("Unable to find user %s(a)%s.\n&quot;),
>  +                user->orig_name, domname);
>  +        return ENOENT;
>  +    }
>  +
>  +    user->domain = newdom;
>  +    return EOK;
>  +}
>  +
>  +static errno_t get_group_domain_msg(struct sss_tool_ctx *tool_ctx,
>  +                                    struct override_group *group)
>  +{
>  +    struct sss_domain_info *newdom;
>  +    const char *domname;
>  +
>  +    newdom = get_object_domain(SYSDB_MEMBER_GROUP, group->orig_name,
>  +                               group->domain, tool_ctx->domains);
>  +    if (newdom == NULL) {
>  +        domname = group->domain == NULL ? "[unknown]" :
> group->domain->name;
>  +        fprintf(stderr, _("Unable to find group %s(a)%s.\n&quot;),
>  +                group->orig_name, domname);
>  +        return ENOENT;
>  +    }
>  +
>  +    group->domain = newdom;
>  +    return EOK;
>   }
>   
>  -static const char * get_group_dn_and_domain(TALLOC_CTX *mem_ctx,
>  -                                            struct sss_domain_info *domains,
>  -                                            struct override_group *group)
>  +static errno_t get_object_dn(TALLOC_CTX *mem_ctx,
>  +                             struct sss_domain_info *domain,
>  +                             enum sysdb_member_type type,
>  +                             const char *name,
>  +                             struct ldb_dn **_ldb_dn,
>  +                             const char **_str_dn)
>   {
>  -    return get_object_dn_and_domain(mem_ctx, SYSDB_MEMBER_GROUP,
>  -                         group->orig_name, group->domain, domains,
>  -                         &group->domain);
>  +    struct ldb_dn *ldb_dn;
>  +
>  +    switch (type) {
>  +    case SYSDB_MEMBER_USER:
>  +       ldb_dn = sysdb_user_dn(mem_ctx, domain, name);
>  +       break;
>  +    case SYSDB_MEMBER_GROUP:
>  +       ldb_dn = sysdb_group_dn(mem_ctx, domain, name);
>  +       break;
>  +    default:
>  +       DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);
>  +       return ERR_INTERNAL;
>  +    }
>  +
>  +    if (ldb_dn == NULL) {
>  +        return ENOMEM;
>  +    }
>  +
>  +    if (_str_dn != NULL) {
>  +        *_str_dn = ldb_dn_get_linearized(ldb_dn);
>  +    }
>  +
>  +    if (_ldb_dn != NULL) {
>  +        *_ldb_dn = ldb_dn;
>  +    } else {
>  +        talloc_free(ldb_dn);
>  +    }
>  +
>  +    return EOK;
>   }
>   
>   static errno_t override_object_add(struct sss_domain_info *domain,
>                                      enum sysdb_member_type type,
>                                      struct sysdb_attrs *attrs,
>  -                                   const char *obj_dn)
>  +                                   const char *name)
>   {
>       TALLOC_CTX *tmp_ctx;
>       const char *anchor;
>       struct ldb_dn *ldb_dn;
>  +    const char *str_dn;
>       errno_t ret;
>   
>       tmp_ctx = talloc_new(NULL);
>  @@ -427,13 +524,12 @@ static errno_t override_object_add(struct sss_domain_info *domain,
>           return ENOMEM;
>       }
>   
>  -    ldb_dn = ldb_dn_new(tmp_ctx, sysdb_ctx_get_ldb(domain->sysdb), obj_dn);
>  -    if (ldb_dn == NULL) {
>  -        ret = ENOMEM;
>  +    ret = get_object_dn(tmp_ctx, domain, type, name, &ldb_dn, &str_dn);
>  +    if (ret != EOK) {
>           goto done;
>       }
>   
>  -    anchor = build_anchor(tmp_ctx, obj_dn);
>  +    anchor = build_anchor(tmp_ctx, str_dn);
>       if (anchor == NULL) {
>           ret = ENOMEM;
>           goto done;
>  @@ -444,7 +540,7 @@ static errno_t override_object_add(struct sss_domain_info *domain,
>           goto done;
>       }
>   
>  -    DEBUG(SSSDBG_TRACE_FUNC, "Creating override for %s\n", obj_dn);
>  +    DEBUG(SSSDBG_TRACE_FUNC, "Creating override for %s\n", str_dn);
>   
>       ret = sysdb_store_override(domain, LOCALVIEW, type, attrs, ldb_dn);
>   
>  @@ -453,13 +549,70 @@ done:
>       return ret;
>   }
>   
>  +static errno_t override_user(struct sss_tool_ctx *tool_ctx,
>  +                             struct override_user *user)
>  +{
>  +    struct sysdb_attrs *attrs;
>  +    errno_t ret;
>  +
>  +    ret = prepare_view_msg(user->domain);
>  +    if (ret != EOK) {
>  +        return ret;
>  +    }
>  +
>  +    attrs = build_user_attrs(tool_ctx, user);
>  +    if (attrs == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to build sysdb attrs.\n");
>  +        return ENOMEM;
>  +    }
>  +
>  +    ret = override_object_add(user->domain, SYSDB_MEMBER_USER, attrs,
>  +                              user->orig_name);
>  +    if (ret != EOK) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to add override object.\n");
>  +        return ret;
>  +    }
>  +
>  +    return EOK;
>  +}
>  +
>  +static errno_t override_group(struct sss_tool_ctx *tool_ctx,
>  +                              struct override_group *group)
>  +{
>  +    struct sysdb_attrs *attrs;
>  +    errno_t ret;
>  +
>  +    ret = prepare_view_msg(group->domain);
>  +    if (ret != EOK) {
>  +        return ret;
>  +    }
>  +
>  +    attrs = build_group_attrs(tool_ctx, group);
>  +    if (attrs == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to build sysdb attrs.\n");
>  +        return ENOMEM;
>  +    }
>  +
>  +    ret = override_object_add(group->domain, SYSDB_MEMBER_GROUP, attrs,
>  +                              group->orig_name);
>  +    if (ret != EOK) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to add override object.\n");
>  +        return ret;
>  +    }
>  +
>  +    return EOK;
>  +}
>  +
>   static errno_t override_object_del(struct sss_domain_info *domain,
>  -                                   const char *obj_dn)
>  +                                   enum sysdb_member_type type,
>  +                                   const char *name)
>   {
>       TALLOC_CTX *tmp_ctx;
>  -    const char *anchor;
>  -    struct ldb_dn *override_dn;
>       struct ldb_message *msg;
>  +    struct ldb_dn *override_dn;
>  +    struct ldb_dn *ldb_dn;
>  +    const char *str_dn;
>  +    const char *anchor;
>       errno_t ret;
>       int sret;
>       bool in_transaction = false;
>  @@ -470,7 +623,12 @@ static errno_t override_object_del(struct sss_domain_info *domain,
>           return ENOMEM;
>       }
>   
>  -    anchor = build_anchor(tmp_ctx, obj_dn);
>  +    ret = get_object_dn(tmp_ctx, domain, type, name, &ldb_dn, &str_dn);
>  +    if (ret != EOK) {
>  +        goto done;
>  +    }
>  +
>  +    anchor = build_anchor(tmp_ctx, str_dn);
>       if (anchor == NULL) {
>           ret = ENOMEM;
>           goto done;
>  @@ -483,7 +641,7 @@ static errno_t override_object_del(struct sss_domain_info *domain,
>           goto done;
>       }
>   
>  -    DEBUG(SSSDBG_TRACE_FUNC, "Removing override for %s\n", obj_dn);
>  +    DEBUG(SSSDBG_TRACE_FUNC, "Removing override for %s\n", str_dn);
>   
>       ret = sysdb_transaction_start(domain->sysdb);
>       if (ret != EOK) {
>  @@ -504,7 +662,7 @@ static errno_t override_object_del(struct sss_domain_info *domain,
>           goto done;
>       }
>   
>  -    msg->dn = ldb_dn_new(msg, ldb, obj_dn);
>  +    msg->dn = talloc_steal(msg, ldb_dn);
>       if (msg->dn == NULL) {
>           ret = ENOMEM;
>           goto done;
>  @@ -547,13 +705,243 @@ done:
>       return ret;
>   }
>   
>  +static errno_t append_name(struct sss_domain_info *domain,
>  +                           struct ldb_message *override)
>  +{
>  +    TALLOC_CTX *tmp_ctx;
>  +    struct ldb_context *ldb = sysdb_ctx_get_ldb(domain->sysdb);
>  +    struct ldb_dn *dn;
>  +    struct ldb_message **msgs;
>  +    const char *attrs[] = {SYSDB_NAME, NULL};
>  +    const char *name;
>  +    size_t count;
>  +    errno_t ret;
>  +
>  +    tmp_ctx = talloc_new(NULL);
>  +    if (tmp_ctx == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n");
>  +        return ENOMEM;
>  +    }
>  +
>  +    dn = ldb_msg_find_attr_as_dn(ldb, tmp_ctx, override,
>  +                                 SYSDB_OVERRIDE_OBJECT_DN);
>  +    if (dn == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Missing overrideObjectDN?\n");
>  +        ret = ERR_INTERNAL;
>  +        goto done;
>  +    }
>  +
>  +    ret = sysdb_search_entry(tmp_ctx, domain->sysdb, dn, LDB_SCOPE_BASE,
>  +                             NULL, attrs, &count, &msgs);
>  +    if (ret != EOK) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "sysdb_search_entry() failed [%d]: %s\n",
>  +              ret, sss_strerror(ret));
>  +        goto done;
>  +    } else if (count != 1) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "More than one user found?\n");
>  +        ret = ERR_INTERNAL;
>  +        goto done;
>  +    }
>  +
>  +    name = ldb_msg_find_attr_as_string(msgs[0], SYSDB_NAME, NULL);
>  +    if (name == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Object with no name?\n");
>  +        ret = ERR_INTERNAL;
>  +        goto done;
>  +    }
>  +
>  +    ret = ldb_msg_add_string(override, ORIGNAME, name);
>  +    if (ret != LDB_SUCCESS) {
>  +        ret = sysdb_error_to_errno(ret);
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to add attribute to msg\n");
>  +        goto done;
>  +    }
>  +
>  +    talloc_steal(override, name);
>  +
>  +done:
>  +    talloc_free(tmp_ctx);
>  +
>  +    return ret;
>  +}
>  +
>  +static errno_t list_overrides(TALLOC_CTX *mem_ctx,
>  +                              const char *filter,
>  +                              const char **attrs,
>  +                              struct sss_domain_info *domain,
>  +                              size_t *_count,
>  +                              struct ldb_message ***_msgs)
>  +{
>  +    TALLOC_CTX *tmp_ctx;
>  +    struct ldb_dn *dn;
>  +    struct ldb_context *ldb = sysdb_ctx_get_ldb(domain->sysdb);
>  +    size_t count;
>  +    struct ldb_message **msgs;
>  +    size_t i;
>  +    int ret;
>  +
>  +    tmp_ctx = talloc_new(NULL);
>  +    if (tmp_ctx == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n");
>  +        return ENOMEM;
>  +    }
>  +
>  +    /* Acquire list of override objects. */
>  +    dn = ldb_dn_new_fmt(tmp_ctx, ldb, SYSDB_TMPL_VIEW_SEARCH_BASE, LOCALVIEW);
>  +    if (dn == NULL) {
>  +        DEBUG(SSSDBG_OP_FAILURE, "ldb_dn_new_fmt() failed.\n");
>  +        ret = EIO;
>  +        goto done;
>  +    }
>  +
>  +    ret = sysdb_search_entry(tmp_ctx, domain->sysdb, dn, LDB_SCOPE_SUBTREE,
>  +                             filter, attrs, &count, &msgs);
>  +    if (ret != EOK) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "sysdb_search_entry() failed [%d]: %s\n",
>  +              ret, sss_strerror(ret));
>  +        goto done;
>  +    }
>  +
>  +    /* Amend messages with original name. */
>  +    for (i = 0; i < count; i++) {
>  +        ret = append_name(domain, msgs[i]);
>  +        if (ret != EOK) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to append name [%d]: %s\n",
>  +                  ret, sss_strerror(ret));
>  +            goto done;
>  +        }
>  +    }
>  +
>  +    *_msgs = talloc_steal(mem_ctx, msgs);
>  +    *_count = count;
>  +
>  +    ret = EOK;
>  +
>  +done:
>  +    talloc_free(tmp_ctx);
>  +
>  +    return ret;
>  +}
>  +
>  +static struct override_user *
>  +list_user_overrides(TALLOC_CTX *mem_ctx,
>  +                    struct sss_domain_info *domain)
>  +{
>  +    TALLOC_CTX *tmp_ctx;
>  +    struct override_user *objs;
>  +    struct ldb_message **msgs;
>  +    size_t count;
>  +    size_t i;
>  +    errno_t ret;
>  +    const char *attrs[] = SYSDB_PW_ATTRS;
>  +
>  +    tmp_ctx = talloc_new(NULL);
>  +    if (tmp_ctx == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n");
>  +        return NULL;
>  +    }
>  +
>  +    ret = list_overrides(tmp_ctx, "(objectClass=" SYSDB_OVERRIDE_USER_CLASS
> ")",
>  +                         attrs, domain, &count, &msgs);
>  +    if (ret != EOK) {
>  +        goto done;
>  +    }
>  +
>  +    objs = talloc_zero_array(tmp_ctx, struct override_user, count + 1);
>  +    if (objs == NULL) {
>  +        ret = ENOMEM;
>  +        goto done;
>  +    }
>  +
>  +    for (i = 0; i < count; i++) {
>  +        objs[i].orig_name = ldb_msg_find_attr_as_string(msgs[i], ORIGNAME,
>  +                                                        NULL);
>  +        if (objs[i].orig_name == NULL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE, "Missing name?!\n");
>  +            ret = ERR_INTERNAL;
>  +            goto done;
>  +        }
>  +
>  +        objs[i].name = ldb_msg_find_attr_as_string(msgs[i], SYSDB_NAME, NULL);
>  +        objs[i].uid = ldb_msg_find_attr_as_uint(msgs[i], SYSDB_UIDNUM, 0);
>  +        objs[i].gid = ldb_msg_find_attr_as_uint(msgs[i], SYSDB_GIDNUM, 0);
>  +        objs[i].home = ldb_msg_find_attr_as_string(msgs[i], SYSDB_HOMEDIR, NULL);
>  +        objs[i].shell = ldb_msg_find_attr_as_string(msgs[i], SYSDB_SHELL, NULL);
>  +        objs[i].gecos = ldb_msg_find_attr_as_string(msgs[i], SYSDB_GECOS, NULL);
>  +    }
>  +
>  +    talloc_steal(mem_ctx, objs);
>  +
>  +done:
>  +    talloc_free(tmp_ctx);
>  +
>  +    if (ret != EOK) {
>  +        return NULL;
>  +    }
>  +
>  +    return objs;
>  +}
>  +
>  +static struct override_group *
>  +list_group_overrides(TALLOC_CTX *mem_ctx,
>  +                     struct sss_domain_info *domain)
>  +{
>  +    TALLOC_CTX *tmp_ctx;
>  +    struct override_group *objs;
>  +    struct ldb_message **msgs;
>  +    size_t count;
>  +    size_t i;
>  +    errno_t ret;
>  +    const char *attrs[] = SYSDB_GRSRC_ATTRS;
>  +
>  +    tmp_ctx = talloc_new(NULL);
>  +    if (tmp_ctx == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n");
>  +        return NULL;
>  +    }
>  +
>  +    ret = list_overrides(tmp_ctx, "(objectClass=" SYSDB_OVERRIDE_GROUP_CLASS
> ")",
>  +                         attrs, domain, &count, &msgs);
>  +    if (ret != EOK) {
>  +        goto done;
>  +    }
>  +
>  +    objs = talloc_zero_array(tmp_ctx, struct override_group, count + 1);
>  +    if (objs == NULL) {
>  +        ret = ENOMEM;
>  +        goto done;
>  +    }
>  +
>  +    for (i = 0; i < count; i++) {
>  +        objs[i].orig_name = ldb_msg_find_attr_as_string(msgs[i], ORIGNAME,
>  +                                                        NULL);
>  +        if (objs[i].orig_name == NULL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE, "Missing name?!\n");
>  +            ret = ERR_INTERNAL;
>  +            goto done;
>  +        }
>  +
>  +        objs[i].name = ldb_msg_find_attr_as_string(msgs[i], SYSDB_NAME, NULL);
>  +        objs[i].gid = ldb_msg_find_attr_as_uint(msgs[i], SYSDB_GIDNUM, 0);
>  +    }
>  +
>  +    talloc_steal(mem_ctx, objs);
>  +
>  +done:
>  +    talloc_free(tmp_ctx);
>  +
>  +    if (ret != EOK) {
>  +        return NULL;
>  +    }
>  +
>  +    return objs;
>  +}
>  +
>   static int override_user_add(struct sss_cmdline *cmdline,
>                                struct sss_tool_ctx *tool_ctx,
>                                void *pvt)
>   {
>       struct override_user user = {NULL};
>  -    struct sysdb_attrs *attrs;
>  -    const char *dn;
>       int ret;
>   
>       ret = parse_cmdline_user_add(cmdline, tool_ctx, &user);
>  @@ -562,34 +950,13 @@ static int override_user_add(struct sss_cmdline *cmdline,
>           return EXIT_FAILURE;
>       }
>   
>  -    dn = get_user_dn_and_domain(tool_ctx, tool_ctx->domains, &user);
>  -    if (dn == NULL) {
>  -        fprintf(stderr, _("Unable to find user %s(a)%s.\n&quot;),
>  -                user.orig_name,
>  -                user.domain == NULL ? "[unknown]" : user.domain->name);
>  -        return EXIT_FAILURE;
>  -    }
>  -
>  -    ret = prepare_view(user.domain);
>  -    if (ret == EEXIST) {
>  -        fprintf(stderr, _("Other than LOCAL view already exist in "
>  -                "domain %s.\n"), user.domain->name);
>  -        return EXIT_FAILURE;
>  -    } else if (ret != EOK) {
>  -        fprintf(stderr, _("Unable to prepare view [%d]: %s.\n"),
>  -                ret, sss_strerror(ret));
>  -        return EXIT_FAILURE;
>  -    }
>  -
>  -    attrs = build_user_attrs(tool_ctx, &user);
>  -    if (attrs == NULL) {
>  -        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to build sysdb attrs.\n");
>  +    ret = get_user_domain_msg(tool_ctx, &user);
>  +    if (ret != EOK) {
>           return EXIT_FAILURE;
>       }
>   
>  -    ret = override_object_add(user.domain, SYSDB_MEMBER_USER, attrs, dn);
>  +    ret = override_user(tool_ctx, &user);
>       if (ret != EOK) {
>  -        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to add override object.\n");
>           return EXIT_FAILURE;
>       }
>   
>  @@ -601,7 +968,6 @@ static int override_user_del(struct sss_cmdline *cmdline,
>                                void *pvt)
>   {
>       struct override_user user = {NULL};
>  -    const char *dn;
>       int ret;
>   
>       ret = parse_cmdline_user_del(cmdline, tool_ctx, &user);
>  @@ -610,29 +976,181 @@ static int override_user_del(struct sss_cmdline *cmdline,
>           return EXIT_FAILURE;
>       }
>   
>  -    dn = get_user_dn_and_domain(tool_ctx, tool_ctx->domains, &user);
>  -    if (dn == NULL) {
>  -        fprintf(stderr, _("Unable to find user %s(a)%s.\n&quot;),
>  -                user.orig_name, user.domain->name);
>  +    ret = get_user_domain_msg(tool_ctx, &user);
>  +    if (ret != EOK) {
>           return EXIT_FAILURE;
>       }
>   
>  -    ret = override_object_del(user.domain, dn);
>  +    ret = override_object_del(user.domain, SYSDB_MEMBER_USER, user.orig_name);
>       if (ret != EOK) {
>  -        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to add override object.\n");
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to delete override object.\n");
>           return EXIT_FAILURE;
>       }
>   
>       return EXIT_SUCCESS;
>   }
>   
>  +static int override_user_import(struct sss_cmdline *cmdline,
>  +                                struct sss_tool_ctx *tool_ctx,
>  +                                void *pvt)
>  +{
>  +    TALLOC_CTX *tmp_ctx;
>  +    struct sss_colondb *db;
>  +    const char *filename;
>  +    struct override_user obj;
>  +    int linenum = 1;
>  +    errno_t ret;
>  +    int exit;
>  +
>  +    /**
>  +     * Format: orig_name:name:uid:gid:gecos:home:shell
>  +     */
>  +    struct sss_colondb_read_field table[] = {
>  +        {SSS_COLONDB_STRING, {.str = &obj.input_name}},
>  +        {SSS_COLONDB_STRING, {.str = &obj.name}},
>  +        {SSS_COLONDB_UINT32, {.uint32 = &obj.uid}},
>  +        {SSS_COLONDB_UINT32, {.uint32 = &obj.gid}},
>  +        {SSS_COLONDB_STRING, {.str = &obj.gecos}},
>  +        {SSS_COLONDB_STRING, {.str = &obj.home}},
>  +        {SSS_COLONDB_STRING, {.str = &obj.shell}},
>  +        {SSS_COLONDB_SENTINEL, {0}}
>  +    };
>  +
>  +    ret = parse_cmdline_import(cmdline, tool_ctx, &filename);
>  +    if (ret != EOK) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    db = sss_colondb_open(tool_ctx, SSS_COLONDB_READ, filename);
>  +    if (db == NULL) {
>  +        fprintf(stderr, _("Unable to open %s.\n"), filename);
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    tmp_ctx = talloc_new(NULL);
>  +    if (tmp_ctx == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    while ((ret = sss_colondb_readline(tmp_ctx, db, table)) == EOK) {
>  +        linenum++;
>  +
>  +        ret = sss_tool_parse_name(tool_ctx, tool_ctx, obj.input_name,
>  +                                  &obj.orig_name, &obj.domain);
>  +        if (ret != EOK) {
>  +            fprintf(stderr, _("Unable to parse name %s.\n"), obj.input_name);
>  +            exit = EXIT_FAILURE;
>  +            goto done;
>  +        }
>  +
>  +        ret = get_user_domain_msg(tool_ctx, &obj);
>  +        if (ret != EOK) {
>  +            exit = EXIT_FAILURE;
>  +            goto done;
>  +        }
>  +
>  +        ret = override_user(tool_ctx, &obj);
>  +        if (ret != EOK) {
>  +            exit = EXIT_FAILURE;
>  +            goto done;
>  +        }
>  +
>  +        talloc_free_children(tmp_ctx);
>  +    }
>  +
>  +    if (ret != EOF) {
>  +        fprintf(stderr, _("Invalid format on line %d. "
>  +                "Use --debug option for more information.\n"), linenum);
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    exit = EXIT_SUCCESS;
>  +
>  +done:
>  +    talloc_free(tmp_ctx);
>  +    return exit;
>  +}
>  +
>  +static int override_user_export(struct sss_cmdline *cmdline,
>  +                                struct sss_tool_ctx *tool_ctx,
>  +                                void *pvt)
>  +{
>  +    struct sss_colondb *db;
>  +    const char *filename;
>  +    struct override_user *objs;
>  +    struct sss_domain_info *dom;
>  +    errno_t ret;
>  +    int exit;
>  +    int i;
>  +
>  +    ret = parse_cmdline_export(cmdline, tool_ctx, &filename);
>  +    if (ret != EOK) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    db = sss_colondb_open(tool_ctx, SSS_COLONDB_WRITE, filename);
>  +    if (db == NULL) {
>  +        fprintf(stderr, _("Unable to open %s.\n"), filename);
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    dom = tool_ctx->domains;
>  +    do {
>  +        objs = list_user_overrides(tool_ctx, tool_ctx->domains);
>  +        if (objs == NULL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get override objects\n");
>  +            exit = EXIT_FAILURE;
>  +            goto done;
>  +        }
>  +
>  +        for (i = 0; objs[i].orig_name != NULL; i++) {
>  +            /**
>  +             * Format: orig_name:name:uid:gid:gecos:home:shell
>  +             */
>  +            struct sss_colondb_write_field table[] = {
>  +                {SSS_COLONDB_STRING, {.str = objs[i].orig_name}},
>  +                {SSS_COLONDB_STRING, {.str = objs[i].name}},
>  +                {SSS_COLONDB_UINT32, {.uint32 = objs[i].uid}},
>  +                {SSS_COLONDB_UINT32, {.uint32 = objs[i].gid}},
>  +                {SSS_COLONDB_STRING, {.str = objs[i].gecos}},
>  +                {SSS_COLONDB_STRING, {.str = objs[i].home}},
>  +                {SSS_COLONDB_STRING, {.str = objs[i].shell}},
>  +                {SSS_COLONDB_SENTINEL, {0}}
>  +            };
>  +
>  +            ret = sss_colondb_writeline(db, table);
>  +            if (ret != EOK) {
>  +                DEBUG(SSSDBG_CRIT_FAILURE, "Unable to write line to db\n");
>  +                exit = EXIT_FAILURE;
>  +                goto done;
>  +            }
>  +        }
>  +
>  +        /* All overrides are under the same subtree, so we don't want to
>  +         * descent into subdomains. */
>  +        dom = get_next_domain(dom, false);
>  +    } while (dom != NULL);
>  +
>  +    exit = EXIT_SUCCESS;
>  +
>  +done:
>  +    return exit;
>  +}
>  +
>   static int override_group_add(struct sss_cmdline *cmdline,
>                                 struct sss_tool_ctx *tool_ctx,
>                                 void *pvt)
>   {
>       struct override_group group = {NULL};
>  -    struct sysdb_attrs *attrs;
>  -    const char *dn;
>       int ret;
>   
>       ret = parse_cmdline_group_add(cmdline, tool_ctx, &group);
>  @@ -641,33 +1159,13 @@ static int override_group_add(struct sss_cmdline *cmdline,
>           return EXIT_FAILURE;
>       }
>   
>  -    dn = get_group_dn_and_domain(tool_ctx, tool_ctx->domains, &group);
>  -    if (dn == NULL) {
>  -        fprintf(stderr, _("Unable to find group %s(a)%s.\n&quot;),
>  -                group.orig_name, group.domain->name);
>  -        return EXIT_FAILURE;
>  -    }
>  -
>  -    ret = prepare_view(group.domain);
>  -    if (ret == EEXIST) {
>  -        fprintf(stderr, _("Other than LOCAL view already exist in "
>  -                "domain %s.\n"), group.domain->name);
>  -        return EXIT_FAILURE;
>  -    } else if (ret != EOK) {
>  -        fprintf(stderr, _("Unable to prepare view [%d]: %s.\n"),
>  -                ret, sss_strerror(ret));
>  -        return EXIT_FAILURE;
>  -    }
>  -
>  -    attrs = build_group_attrs(tool_ctx, &group);
>  -    if (attrs == NULL) {
>  -        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to build sysdb attrs.\n");
>  +    ret = get_group_domain_msg(tool_ctx, &group);
>  +    if (ret != EOK) {
>           return EXIT_FAILURE;
>       }
>   
>  -    ret = override_object_add(group.domain, SYSDB_MEMBER_GROUP, attrs, dn);
>  +    ret = override_group(tool_ctx, &group);
>       if (ret != EOK) {
>  -        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to add override object.\n");
>           return EXIT_FAILURE;
>       }
>   
>  @@ -679,7 +1177,6 @@ static int override_group_del(struct sss_cmdline *cmdline,
>                                 void *pvt)
>   {
>       struct override_group group = {NULL};
>  -    const char *dn;
>       int ret;
>   
>       ret = parse_cmdline_group_del(cmdline, tool_ctx, &group);
>  @@ -688,29 +1185,180 @@ static int override_group_del(struct sss_cmdline *cmdline,
>           return EXIT_FAILURE;
>       }
>   
>  -    dn = get_group_dn_and_domain(tool_ctx, tool_ctx->domains, &group);
>  -    if (dn == NULL) {
>  -        fprintf(stderr, _("Unable to find group %s(a)%s.\n&quot;),
>  -                group.orig_name, group.domain->name);
>  +    ret = get_group_domain_msg(tool_ctx, &group);
>  +    if (ret != EOK) {
>           return EXIT_FAILURE;
>       }
>   
>  -    ret = override_object_del(group.domain, dn);
>  +    ret = override_object_del(group.domain, SYSDB_MEMBER_GROUP,
>  +                              group.orig_name);
>       if (ret != EOK) {
>  -        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to add override object.\n");
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to delete override object.\n");
>           return EXIT_FAILURE;
>       }
>   
>       return EXIT_SUCCESS;
>   }
>   
>  +static int override_group_import(struct sss_cmdline *cmdline,
>  +                                 struct sss_tool_ctx *tool_ctx,
>  +                                 void *pvt)
>  +{
>  +    TALLOC_CTX *tmp_ctx;
>  +    struct sss_colondb *db;
>  +    const char *filename;
>  +    struct override_group obj;
>  +    int linenum = 1;
>  +    errno_t ret;
>  +    int exit;
>  +
>  +    /**
>  +     * Format: orig_name:name:gid
>  +     */
>  +    struct sss_colondb_read_field table[] = {
>  +        {SSS_COLONDB_STRING, {.str = &obj.input_name}},
>  +        {SSS_COLONDB_STRING, {.str = &obj.name}},
>  +        {SSS_COLONDB_UINT32, {.uint32 = &obj.gid}},
>  +        {SSS_COLONDB_SENTINEL, {0}}
>  +    };
>  +
>  +    ret = parse_cmdline_import(cmdline, tool_ctx, &filename);
>  +    if (ret != EOK) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    db = sss_colondb_open(tool_ctx, SSS_COLONDB_READ, filename);
>  +    if (db == NULL) {
>  +        fprintf(stderr, _("Unable to open %s.\n"), filename);
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    tmp_ctx = talloc_new(NULL);
>  +    if (tmp_ctx == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    while ((ret = sss_colondb_readline(tmp_ctx, db, table)) == EOK) {
>  +        linenum++;
>  +
>  +        ret = sss_tool_parse_name(tool_ctx, tool_ctx, obj.input_name,
>  +                                  &obj.orig_name, &obj.domain);
>  +        if (ret != EOK) {
>  +            fprintf(stderr, _("Unable to parse name %s.\n"), obj.input_name);
>  +            exit = EXIT_FAILURE;
>  +            goto done;
>  +        }
>  +
>  +        ret = get_group_domain_msg(tool_ctx, &obj);
>  +        if (ret != EOK) {
>  +            exit = EXIT_FAILURE;
>  +            goto done;
>  +        }
>  +
>  +        ret = override_group(tool_ctx, &obj);
>  +        if (ret != EOK) {
>  +            exit = EXIT_FAILURE;
>  +            goto done;
>  +        }
>  +
>  +        talloc_free_children(tmp_ctx);
>  +    }
>  +
>  +    if (ret != EOF) {
>  +        fprintf(stderr, _("Invalid format on line %d. "
>  +                "Use --debug option for more information.\n"), linenum);
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    exit = EXIT_SUCCESS;
>  +
>  +done:
>  +    talloc_free(tmp_ctx);
>  +    return exit;
>  +}
>  +
>  +static int override_group_export(struct sss_cmdline *cmdline,
>  +                                 struct sss_tool_ctx *tool_ctx,
>  +                                 void *pvt)
>  +{
>  +    struct sss_colondb *db;
>  +    const char *filename;
>  +    struct override_group *objs;
>  +    struct sss_domain_info *dom;
>  +    errno_t ret;
>  +    int exit;
>  +    int i;
>  +
>  +    ret = parse_cmdline_export(cmdline, tool_ctx, &filename);
>  +    if (ret != EOK) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    db = sss_colondb_open(tool_ctx, SSS_COLONDB_WRITE, filename);
>  +    if (db == NULL) {
>  +        fprintf(stderr, _("Unable to open %s.\n"), filename);
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    dom = tool_ctx->domains;
>  +    do {
>  +        objs = list_group_overrides(tool_ctx, tool_ctx->domains);
>  +        if (objs == NULL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get override objects\n");
>  +            exit = EXIT_FAILURE;
>  +            goto done;
>  +        }
>  +
>  +        for (i = 0; objs[i].orig_name != NULL; i++) {
>  +            /**
>  +             * Format: orig_name:name:uid:gid:gecos:home:shell
>  +             */
>  +            struct sss_colondb_write_field table[] = {
>  +                {SSS_COLONDB_STRING, {.str = objs[i].orig_name}},
>  +                {SSS_COLONDB_STRING, {.str = objs[i].name}},
>  +                {SSS_COLONDB_UINT32, {.uint32 = objs[i].gid}},
>  +                {SSS_COLONDB_SENTINEL, {0}}
>  +            };
>  +
>  +            ret = sss_colondb_writeline(db, table);
>  +            if (ret != EOK) {
>  +                DEBUG(SSSDBG_CRIT_FAILURE, "Unable to write line to db\n");
>  +                exit = EXIT_FAILURE;
>  +                goto done;
>  +            }
>  +        }
>  +
>  +        /* All overrides are under the same subtree, so we don't want to
>  +         * descent into subdomains. */
>  +        dom = get_next_domain(dom, false);
>  +    } while (dom != NULL);
>  +
>  +    exit = EXIT_SUCCESS;
>  +
>  +done:
>  +    return exit;
>  +}
>  +
>   int main(int argc, const char **argv)
>   {
>       struct sss_route_cmd commands[] = {
>           {"user-add", override_user_add},
>           {"user-del", override_user_del},
>  +        {"user-import", override_user_import},
>  +        {"user-export", override_user_export},
>           {"group-add", override_group_add},
>           {"group-del", override_group_del},
>  +        {"group-import", override_group_import},
>  +        {"group-export", override_group_export},
>           {NULL, NULL}
>       };
>   
>  -- 
>  1.9.3
>   
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

On 08/17/2015 10:32 AM, Jakub Hrozek wrote:
>  On Sun, Aug 16, 2015 at 05:59:22PM +0200, 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.
>
>  Thanks a lot for the patches. I didn't do any review yet, just scrolled
>  through them.
>
>  My first reaction was that the second patch should be split up and at
>  least the colondb should be unit tested. 
Ok. Do you have any specific split on mind?

>  btw did you get inspired by some existing code, like libuser or
>  shadow-utils with the colondb? 
Nope.

[Lukas Slebodnik]

On (17/08/15 10:32), Jakub Hrozek wrote:
> On Sun, Aug 16, 2015 at 05:59:22PM +0200, 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.
>
> Thanks a lot for the patches. I didn't do any review yet, just scrolled
> through them.
>
> My first reaction was that the second patch should be split up and at
> least the colondb should be unit tested.
>
> btw did you get inspired by some existing code, like libuser or
> shadow-utils with the colondb?
>  
Is there a design page for this?
Maybe I missed something but I haven't noticed any discussion about
export format for "local views".

I'm not very happy with custom format. Yes, it's already late
because patches are ready and deadline is comming.

Did you consider a ldif format?

libldb already provide such functions.
[alcik@unused-4-233][~/projects/sssd]$objdump -T /usr/lib64/libldb.so | grep ldb_ldif
000000000000afe0 g    DF .text  0000000000000641  LDB_1.1.0   ldb_ldif_parse_modrdn
000000000000afc0 g    DF .text  0000000000000008  LDB_0.9.10  ldb_ldif_write
000000000000bcc0 g    DF .text  000000000000000f  LDB_1.1.5   ldb_ldif_read_file_state
000000000000be00 g    DF .text  0000000000000019  LDB_0.9.10  ldb_ldif_message_string
000000000000bd40 g    DF .text  0000000000000054  LDB_1.1.12 
ldb_ldif_write_redacted_trace_string
000000000000bcd0 g    DF .text  0000000000000015  LDB_0.9.10  ldb_ldif_read_file
000000000000bd20 g    DF .text  000000000000001f  LDB_0.9.10  ldb_ldif_write_file
000000000000b630 g    DF .text  0000000000000683  LDB_0.9.10  ldb_ldif_read
000000000000afd0 g    DF .text  000000000000000f  LDB_0.9.10  ldb_ldif_read_free
000000000000bda0 g    DF .text  0000000000000054  LDB_0.9.10  ldb_ldif_write_string
000000000000bcf0 g    DF .text  000000000000002b  LDB_0.9.10  ldb_ldif_read_string

e.g.
ldb_ldif_message_string can be used to format "struct ldb_message" as string
/*
   Produce a string form of an ldb message

   convenient function to turn a ldb_message into a string. Useful for
   debugging
 */
char *ldb_ldif_message_string(struct ldb_context *ldb, TALLOC_CTX *mem_ctx,
                              enum ldb_changetype changetype,
                              const struct ldb_message *msg);

LS

[Pavel Březina]

On 08/17/2015 10:52 AM, Lukas Slebodnik wrote:
>  On (17/08/15 10:32), Jakub Hrozek wrote:
> > On Sun, Aug 16, 2015 at 05:59:22PM +0200, 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.
> >
> > Thanks a lot for the patches. I didn't do any review yet, just scrolled
> > through them.
> >
> > My first reaction was that the second patch should be split up and at
> > least the colondb should be unit tested.
> >
> > btw did you get inspired by some existing code, like libuser or
> > shadow-utils with the colondb?
> >
>  Is there a design page for this?
>  Maybe I missed something but I haven't noticed any discussion about
>  export format for "local views".
>
>  I'm not very happy with custom format. Yes, it's already late
>  because patches are ready and deadline is comming. 
The format is compatible with Dell VAS which is used by customer who 
requested local overrides.

>  Did you consider a ldif format?
>
>  libldb already provide such functions.
>  [alcik@unused-4-233][~/projects/sssd]$objdump -T /usr/lib64/libldb.so | grep ldb_ldif
>  000000000000afe0 g    DF .text  0000000000000641  LDB_1.1.0   ldb_ldif_parse_modrdn
>  000000000000afc0 g    DF .text  0000000000000008  LDB_0.9.10  ldb_ldif_write
>  000000000000bcc0 g    DF .text  000000000000000f  LDB_1.1.5   ldb_ldif_read_file_state
>  000000000000be00 g    DF .text  0000000000000019  LDB_0.9.10  ldb_ldif_message_string
>  000000000000bd40 g    DF .text  0000000000000054  LDB_1.1.12 
> ldb_ldif_write_redacted_trace_string
>  000000000000bcd0 g    DF .text  0000000000000015  LDB_0.9.10  ldb_ldif_read_file
>  000000000000bd20 g    DF .text  000000000000001f  LDB_0.9.10  ldb_ldif_write_file
>  000000000000b630 g    DF .text  0000000000000683  LDB_0.9.10  ldb_ldif_read
>  000000000000afd0 g    DF .text  000000000000000f  LDB_0.9.10  ldb_ldif_read_free
>  000000000000bda0 g    DF .text  0000000000000054  LDB_0.9.10  ldb_ldif_write_string
>  000000000000bcf0 g    DF .text  000000000000002b  LDB_0.9.10  ldb_ldif_read_string
>
>  e.g.
>  ldb_ldif_message_string can be used to format "struct ldb_message" as string
>  /*
>      Produce a string form of an ldb message
>
>      convenient function to turn a ldb_message into a string. Useful for
>      debugging
>    */
>  char *ldb_ldif_message_string(struct ldb_context *ldb, TALLOC_CTX *mem_ctx,
>                                 enum ldb_changetype changetype,
>                                 const struct ldb_message *msg);
>
>  LS

[Jakub Hrozek]

On Mon, Aug 17, 2015 at 10:52:41AM +0200, Lukas Slebodnik wrote:
>  On (17/08/15 10:32), Jakub Hrozek wrote:
>  >On Sun, Aug 16, 2015 at 05:59:22PM +0200, 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.
>  >
>  >Thanks a lot for the patches. I didn't do any review yet, just scrolled
>  >through them.
>  >
>  >My first reaction was that the second patch should be split up and at
>  >least the colondb should be unit tested.
>  >
>  >btw did you get inspired by some existing code, like libuser or
>  >shadow-utils with the colondb?
>  >
>  Is there a design page for this?
>  Maybe I missed something but I haven't noticed any discussion about
>  export format for "local views". 
Maybe in future we could add support for extra formats, but the first
iterations should use the format as described in man 5 passwd (except we
don't override passwords) and man 5 groups (except we don't override
group members).

[Jakub Hrozek]

On Mon, Aug 17, 2015 at 10:52:41AM +0200, Lukas Slebodnik wrote:
>  On (17/08/15 10:32), Jakub Hrozek wrote:
>  >On Sun, Aug 16, 2015 at 05:59:22PM +0200, 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.
>  >
>  >Thanks a lot for the patches. I didn't do any review yet, just scrolled
>  >through them.
>  >
>  >My first reaction was that the second patch should be split up and at
>  >least the colondb should be unit tested.
>  >
>  >btw did you get inspired by some existing code, like libuser or
>  >shadow-utils with the colondb?
>  >
>  Is there a design page for this?
>  Maybe I missed something but I haven't noticed any discussion about
>  export format for "local views".
>  
>  I'm not very happy with custom format. Yes, it's already late
>  because patches are ready and deadline is comming. 
btw you're right there was no design page. But basically, we should
support this:
    https://support.software.dell.com/authentication-services/kb/91145

[Jakub Hrozek]

On Mon, Aug 17, 2015 at 10:32:00AM +0200, Jakub Hrozek wrote:
>  On Sun, Aug 16, 2015 at 05:59:22PM +0200, 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.
>  
>  Thanks a lot for the patches. I didn't do any review yet, just scrolled
>  through them.
>  
>  My first reaction was that the second patch should be split up and at
>  least the colondb should be unit tested.
>  
>  btw did you get inspired by some existing code, like libuser or
>  shadow-utils with the colondb? 
The patches are quite big, so maybe the review will be delivered in
chunks.

1) There are some Coverity errors:
Error: COMPILER_WARNING:
sssd-1.13.1/src/tools/common/sss_colondb.c:279:12: warning: 'fmode' may be used
uninitialized in this function [-Wmaybe-uninitialized]
#         fp = fopen(filename, fmode);
#            ^
#  277|       if (filename != NULL) {
#  278|           errno = 0;
#  279|->         fp = fopen(filename, fmode);
#  280|           if (fp == NULL) {
#  281|               DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s\n",
filename);

Error: RESOURCE_LEAK (CWE-772):
sssd-1.13.1/src/tools/common/sss_colondb.c:279: alloc_fn: Storage is returned from
allocation function "fopen".
sssd-1.13.1/src/tools/common/sss_colondb.c:279: var_assign: Assigning: "fp" =
storage returned from "fopen(filename, fmode)".
sssd-1.13.1/src/tools/common/sss_colondb.c:289: leaked_storage: Variable "fp"
going out of scope leaks the storage it points to.
#  287|       if (db == NULL) {
#  288|           DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero() failed\n");
#  289|->         return NULL;
#  290|       }
#  291|   

Error: COMPILER_WARNING:
sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
'sss_colondb_open'
sssd-1.13.1/src/tools/common/sss_colondb.c:292:14: warning: 'fp' may be used
uninitialized in this function [-Wmaybe-uninitialized]
#     db->file = fp;
#              ^
#  290|       }
#  291|   
#  292|->     db->file = fp;
#  293|       db->mode = mode;
#  294|   

Error: UNINIT (CWE-457):
sssd-1.13.1/src/tools/sss_override.c:997: var_decl: Declaring variable "tmp_ctx"
without initializer.
sssd-1.13.1/src/tools/sss_override.c:1076: uninit_use_in_call: Using uninitialized value
"tmp_ctx" when calling "_talloc_free".
# 1074|   
# 1075|   done:
# 1076|->     talloc_free(tmp_ctx);
# 1077|       return exit;
# 1078|   }

Error: UNINIT (CWE-457):
sssd-1.13.1/src/tools/sss_override.c:1207: var_decl: Declaring variable
"tmp_ctx" without initializer.
sssd-1.13.1/src/tools/sss_override.c:1282: uninit_use_in_call: Using uninitialized value
"tmp_ctx" when calling "_talloc_free".
# 1280|   
# 1281|   done:
# 1282|->     talloc_free(tmp_ctx);
# 1283|       return exit;
# 1284|   }

2) There is no documentation. It would be nice to have some examples in
the man page also.

3) I should probably test this before asking, but do the import and
export commands support blank (skipped) fields?

[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?= <pbrezina(a)redhat.com&gt;
> 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 <pbrezina(a)redhat.com&gt;
> +
> +    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

[Jakub Hrozek]

On Mon, Aug 17, 2015 at 12:17:29PM +0200, Lukas Slebodnik wrote:
>  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?= <pbrezina(a)redhat.com&gt;
>  >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 <pbrezina(a)redhat.com&gt;
>  >+
>  >+    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. 
I wonder if we could use the macro later:
    30 static char *read_field_as_string(char *line,
    31                                   const char **_value)
    32 {
    33     char *rest;
    34     char *value;
    35
    36     if (line == NULL || *line == '\0' || *line == '\n') {
---> HERE
    37         /* There is nothing else to read. */

>            Side note: if there is expectation occurrence '\n'
> should be higher
>            that occurrence of '\0' then you should change order in condition.
Yes, this wouldn't hurt.

>  
>  //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 
+1

>  >+            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 
Later in the code we have:

>  >+    /* Remove starting : */
>  >+    line++;

[Pavel Březina]

On 08/18/2015 01:23 PM, Jakub Hrozek wrote:
>  On Mon, Aug 17, 2015 at 12:17:29PM +0200, Lukas Slebodnik wrote:
> > 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?= <pbrezina(a)redhat.com&gt;
> >> 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 <pbrezina(a)redhat.com&gt;
> >> +
> >> +    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.
>
>  I wonder if we could use the macro later:
>       30 static char *read_field_as_string(char *line,
>       31                                   const char **_value)
>       32 {
>       33     char *rest;
>       34     char *value;
>       35
>       36     if (line == NULL || *line == '\0' || *line == '\n') {
>  ---> HERE 
I originally had it on two places and forgot to remove it...

>       37         /* There is nothing else to read. */
>
> >            Side note: if there is expectation occurrence '\n' should be
> higher
> >            that occurrence of '\0' then you should change order in
> condition.
>
>  Yes, this wouldn't hurt.
>
> >
> > //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
>
>  +1
>
> >> +            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
>
>  Later in the code we have:
>
> >> +    /* Remove starting : */
> >> +    line++;

[Jakub Hrozek]

On Sun, Aug 16, 2015 at 05:59:22PM +0200, 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. 
>  From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  Date: Sat, 15 Aug 2015 13:53:25 +0200
>  Subject: [PATCH 1/2] sss_override: print input name if unable to parse it 
ACK 

>  From 24655f677acf9f64201a611ababdcfeff4317037 Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  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 
Apart from previous comments Lukas and I had, here are comments abour
colondb:

>  +++ b/src/tools/common/sss_colondb.c
>  @@ -0,0 +1,298 @@
>  +/*
>  +    Authors:
>  +        Pavel Březina <pbrezina(a)redhat.com&gt;
>  +
>  +    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')
>  +
>  +static char *read_field_as_string(char *line,
>  +                                  const char **_value)
>  +{
>  +    char *rest;
>  +    char *value;
>  +
>  +    if (line == NULL || *line == '\0' || *line == '\n') {
>  +        /* There is nothing else to read. */
>  +        rest = NULL;
>  +        value = NULL;
>  +        goto done;
>  +    }
>  +
>  +    if (*line == ':') {
>  +        /* Special case for empty value. */
>  +        *line = '\0';
>  +        rest = line + 1;
>  +        value = NULL;
>  +        goto done;
>  +    }
>  +
>  +    /* Value starts at current position. */
>  +    value = line;
>  +
>  +    /* Find next field delimiter. */
>  +    rest = strchr(line, ':');
>  +    if (rest == NULL) {
>  +        /* There is no more field. Remove \n from the end. */
>  +        rest = strchr(line, '\n');
>  +        if (rest != NULL) {
>  +            *rest = '\0';
>  +            rest = NULL;
>  +        }
>  +        goto done;
>  +    }
>  +
>  +    /* Remove it and step one character further. */
>  +    *rest = '\0';
>  +    rest++;
>  +
>  +done:
>  +    *_value = value;
>  +
>  +    return rest;
>  +}
>  +
>  +static char *read_field_as_uint32(char *line,
>  +                                  uint32_t *_value)
>  +{
>  +    const char *str;
>  +    char *rest;
>  +    errno_t ret;
>  +
>  +    rest = read_field_as_string(line, &str);
>  +    if (rest == NULL || str == NULL) {
>  +        *_value = 0;
>  +        return rest;
>  +    }
>  +
>  +    *_value = strtouint32(str, NULL, 10);
>  +    if (errno != 0) {
>  +        ret = errno;
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse number [%d]: %s\n",
>  +              ret, sss_strerror(ret));
>  +
>  +        *_value = 0;
>  +    }
>  +
>  +    return rest;
>  +}
>  +
>  +struct sss_colondb {
>  +    FILE *file;
>  +    enum sss_colondb_mode mode;
>  +};
>  +
>  +errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
>  +                             struct sss_colondb *db,
>  +                             struct sss_colondb_read_field *table)
>  +{
>  +    int readchars;
>  +    size_t linelen = 0;
>  +    char *line = NULL;
>  +    char *rest;
>  +    errno_t ret;
>  +    int i;
>  +
>  +    if (db->mode != SSS_COLONDB_READ) {
>  +        return ERR_INTERNAL;
>  +    }
>  +
>  +    readchars = getline(&line, &linelen, db->file);
>  +    if (readchars == -1) {
>  +        /* Nothing was read. */
>  +        if (errno != 0) {
>  +            ret = errno;
>  +            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]: %s",
>  +                  ret, sss_strerror(ret));
>  +            return ret;
>  +        }
>  +
>  +        return EOF;
>  +    }
>  +
>  +    /* Copy line to mem_ctx. */
>  +    rest = talloc_strdup(mem_ctx, line);
>  +    if (rest == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup() failed\n");
>  +        return ENOMEM;
>  +    }
>  +    free(line);
>  +    line = rest; 
I don't like reusing variables like this. I would prefer to free() line
after calling strudup, maybe even set it to NULL and then not use it
anymore.

>  +
>  +    for (i = 0; table[i].type != SSS_COLONDB_SENTINEL; i++) {
>  +        switch (table[i].type) {
>  +        case SSS_COLONDB_UINT32:
>  +            rest = read_field_as_uint32(rest, table[i].data.uint32);
>  +            break;
>  +        case SSS_COLONDB_STRING:
>  +            rest = read_field_as_string(rest, table[i].data.str);
>  +            break;
>  +        case SSS_COLONDB_SENTINEL:
>  +            DEBUG(SSSDBG_CRIT_FAILURE, "Trying to process sentinel?!\n");
>  +            ret = ERR_INTERNAL;
>  +            goto done; 
Do you think it would be more fail-save to add default here?

>  +        }
>  +
>  +        if (rest == NULL && table[i + 1].type != SSS_COLONDB_SENTINEL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE,
>  +                  "Line contains less values than expected!\n");
>  +            ret = EINVAL;
>  +            goto done;
>  +        } else if (rest != NULL && table[i + 1].type == SSS_COLONDB_SENTINEL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE,
>  +                  "Line contains more values than expected!\n");
>  +            ret = EINVAL;
>  +            goto done;
>  +        }
>  +    }
>  +
>  +    ret = EOK;
>  +
>  +done:
>  +    if (ret != EOK) {
>  +        talloc_free(line); 
See above about line vs. rest.

>  +    }
>  +
>  +    return ret;
>  +}
>  + 
[...]

>  +struct sss_colondb *sss_colondb_open(TALLOC_CTX *mem_ctx,
>  +                                     enum sss_colondb_mode mode,
>  +                                     const char *filename)
>  +{
>  +    FILE *fp;
>  +    const char *fmode;
>  +    struct sss_colondb *db;
>  +
>  +    switch (mode) {
>  +    case SSS_COLONDB_READ:
>  +        fmode = "r";
>  +        if (filename == NULL) {
>  +            fp = stdin;
>  +        }
>  +        break;
>  +    case SSS_COLONDB_WRITE:
>  +        fmode = "w";
>  +        if (filename == NULL) {
>  +            fp = stdout;
>  +        }
>  +        break;
>  +    }
>  +
>  +    if (filename != NULL) {
>  +        errno = 0;
>  +        fp = fopen(filename, fmode);
>  +        if (fp == NULL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s\n", filename);
>  +            return NULL;
>  +        }
>  +    } 
fp is created either in the switch with fopen. I think it would be nicer
if there was a single function whose responsibility would be to return
fp based on (filename, fmode)

>  +
>  +    db = talloc_zero(mem_ctx, struct sss_colondb);
>  +    if (db == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero() failed\n");
>  +        return NULL;
>  +    }
>  +
>  +    db->file = fp;
>  +    db->mode = mode;
>  +
>  +    talloc_set_destructor((TALLOC_CTX *)db, sss_colondb_close);
>  +
>  +    return db;
>  +}
>  diff --git a/src/tools/common/sss_colondb.h b/src/tools/common/sss_colondb.h
>  new file mode 100644
>  index 0000000000000000000000000000000000000000..13e9c010bb6a233a5f939b453f80b29622fff841
>  --- /dev/null
>  +++ b/src/tools/common/sss_colondb.h
>  @@ -0,0 +1,71 @@
>  +/*
>  +    Authors:
>  +        Pavel Březina <pbrezina(a)redhat.com&gt;
>  +
>  +    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/>;.
>  +*/
>  +
>  +#ifndef _SSS_COLONDB_H_
>  +#define _SSS_COLONDB_H_
>  +
>  +#include <stdlib.h>
>  +#include <talloc.h> 
Please also include stdint and errno so that the header is
self-contained.

>  +
>  +struct sss_colondb;
>  +
>  +enum sss_colondb_mode {
>  +    SSS_COLONDB_READ,
>  +    SSS_COLONDB_WRITE
>  +};
>  +
>  +enum sss_colondb_type {
>  +    SSS_COLONDB_UINT32,
>  +    SSS_COLONDB_STRING,
>  +    SSS_COLONDB_SENTINEL
>  +};
>  +
>  +union sss_colondb_write_data {
>  +    uint32_t uint32;
>  +    const char *str;
>  +};
>  +
>  +union sss_colondb_read_data {
>  +    uint32_t *uint32;
>  +    const char **str;
>  +};
>  +
>  +struct sss_colondb_write_field {
>  +    enum sss_colondb_type type;
>  +    union sss_colondb_write_data data;
>  +};
>  +
>  +struct sss_colondb_read_field {
>  +    enum sss_colondb_type type;
>  +    union sss_colondb_read_data data;
>  +};
>  +
>  +struct sss_colondb *sss_colondb_open(TALLOC_CTX *mem_ctx,
>  +                                     enum sss_colondb_mode mode,
>  +                                     const char *filename);
>  +
>  +errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
>  +                             struct sss_colondb *db,
>  +                             struct sss_colondb_read_field *table); 
I wonder if it would be cleaner if there was a function that would
accept a mem_ctx and line "syntax" and would create a
sss_colondb_read_field* describing a line. Then sss_colondb_readline()
would remove the talloc children first, then fill new data.

Or even make the line syntax part of open?

The usage could look like this:
    db = sss_colondb_open(mem_ctx, SSS_COLONDB_READ, "u:s:s:u");
    whle (sss_colodb_readline(db, &line)) {
    }

I think it would be nicer to have the line syntax part of open, because
the line syntax can't change, it's a property of the file.

>  +
>  +errno_t sss_colondb_writeline(struct sss_colondb *db,
>  +                              struct sss_colondb_write_field *table);
>  +
>  +#endif /* _SSS_COLONDB_H_ */ 
I will send sss_override.c review later.

[Pavel Březina]

On 08/18/2015 01:35 PM, Jakub Hrozek wrote:
>  I wonder if it would be cleaner if there was a function that would
>  accept a mem_ctx and line "syntax" and would create a
>  sss_colondb_read_field* describing a line. Then sss_colondb_readline()
>  would remove the talloc children first, then fill new data.
>
>  Or even make the line syntax part of open?
>
>  The usage could look like this:
>       db = sss_colondb_open(mem_ctx, SSS_COLONDB_READ, "u:s:s:u");
>       whle (sss_colodb_readline(db, &line)) {
>       }
>
>  I think it would be nicer to have the line syntax part of open, because
>  the line syntax can't change, it's a property of the file. 
This is nice. Shall I create a ticket for it? I'd rather get those 
patches pushed and move to fqdn fix first, since this may take longer 
time to get done right.

[Jakub Hrozek]

On Tue, Aug 18, 2015 at 05:15:02PM +0200, Pavel Březina wrote:
>  On 08/18/2015 01:35 PM, Jakub Hrozek wrote:
>  >I wonder if it would be cleaner if there was a function that would
>  >accept a mem_ctx and line "syntax" and would create a
>  >sss_colondb_read_field* describing a line. Then sss_colondb_readline()
>  >would remove the talloc children first, then fill new data.
>  >
>  >Or even make the line syntax part of open?
>  >
>  >The usage could look like this:
>  >     db = sss_colondb_open(mem_ctx, SSS_COLONDB_READ, "u:s:s:u");
>  >     whle (sss_colodb_readline(db, &line)) {
>  >     }
>  >
>  >I think it would be nicer to have the line syntax part of open, because
>  >the line syntax can't change, it's a property of the file.
>  
>  This is nice. Shall I create a ticket for it? I'd rather get those patches
>  pushed and move to fqdn fix first, since this may take longer time to get
>  done right. 
Sure, create a ticket, self-assign it and put it into 1.13.2.

[Jakub Hrozek]

On Sun, Aug 16, 2015 at 05:59:22PM +0200, 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. 
>  From 24655f677acf9f64201a611ababdcfeff4317037 Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  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 
this review is about sss_override.c:

    - please add a man page explanation, ideally with examples of the
      import/export files
    - we need to fix the FQDNs. I would also prefer to print FQDNs on
      export
    - please warn that sssd must be restarted after import
    - as an additional fix, we should support UPNs as well in the first
      column. See the Dell specification - that's what they use. In many
      cases, the FQDN will be the same as UPN, but not always. But I
      don't want to block the inclusion over that.

[Pavel Březina]

On 08/18/2015 03:21 PM, Jakub Hrozek wrote:
>  On Sun, Aug 16, 2015 at 05:59:22PM +0200, 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.
>
> >  From 24655f677acf9f64201a611ababdcfeff4317037 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> > 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
>
>  this review is about sss_override.c:
>
>       - please add a man page explanation, ideally with examples of the
>         import/export files
>       - we need to fix the FQDNs. I would also prefer to print FQDNs on
>         export 
Yes, I'll do it in separate patch since there is a ticket for that. (2757)

BTW fq names works correctly, unless you have use_fully_qualified_names 
= True.

>       - please warn that sssd must be restarted after import 
This works for me with this version.

>       - as an additional fix, we should support UPNs as well in the
> first
>         column. See the Dell specification - that's what they use. In many
>         cases, the FQDN will be the same as UPN, but not always. But I
>         don't want to block the inclusion over that. 
Will do with 2757.

[Pavel Březina]

On 08/16/2015 05:59 PM, 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. 
New patches attached.

[Pavel Březina]

On 08/18/2015 05:24 PM, Pavel Březina wrote:
>  On 08/16/2015 05:59 PM, 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.
>
>  New patches attached. 
I split it into more patches.

[Jakub Hrozek]

On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
>  On 08/18/2015 05:24 PM, Pavel Březina wrote:
>  >On 08/16/2015 05:59 PM, 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.
>  >
>  >New patches attached.
>  
>  I split it into more patches.
>   
There is a strange ordering between this patch and the FQDN -- these
patches can't be compiled without the FQDN one, but the FQDN doesn't
apply on master.

There are still some Coverity errors:
Error: COMPILER_WARNING:
sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
'sss_colondb_open'
sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may be used
uninitialized in this function [-Wmaybe-uninitialized]
#     if (fp == NULL && filename != NULL) {
#            ^
sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was declared here
#     FILE *fp;
#           ^
#  271|       }
#  272|   
#  273|->     if (fp == NULL && filename != NULL) {
#  274|           ret = errno;
#  275|           DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s [%d]:
%s\n",

Error: NEGATIVE_RETURNS (CWE-394):
sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
"sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a negative
number.
sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant: Explicitly returning
negative value "-22".
sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning: unsigned variable
"fqlen" = "sss_fqname".
sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned variable
"fqlen" is incremented, which might cause an integer overflow.
#  340|       fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
#  341|       if (fqlen > 0) {
#  342|->         fqlen++; /* \0 */
#  343|       } else {
#  344|           return NULL;

>  From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  Date: Sat, 15 Aug 2015 13:53:25 +0200
>  Subject: [PATCH 1/5] sss_override: print input name if unable to parse it 
ACK


>  From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  Date: Wed, 19 Aug 2015 12:34:08 +0200
>  Subject: [PATCH 2/5] TOOLS: add sss_colondb API
>  
>  To simplify import/export users and groups. 
More or less ack :-) see some questions inline

>  +errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
>  +                             struct sss_colondb *db,
>  +                             struct sss_colondb_read_field *table)
>  +{
>  +    int readchars;
>  +    size_t linelen = 0;
>  +    char *line = NULL;
>  +    char *tcline;
>  +    char *rest;
>  +    errno_t ret;
>  +    int i;
>  +
>  +    if (db->mode != SSS_COLONDB_READ) {
>  +        return ERR_INTERNAL;
>  +    }
>  +
>  +    readchars = getline(&line, &linelen, db->file);
>  +    if (readchars == -1) {
>  +        /* Nothing was read. */
>  +        if (errno != 0) {
>  +            ret = errno;
>  +            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]: %s",
>  +                  ret, sss_strerror(ret));
>  +            return ret;
>  +        }
>  +
>  +        return EOF;
>  +    }
>  +
>  +    /* Copy line to mem_ctx. */
>  +    tcline = talloc_strdup(mem_ctx, line); 
Do we need mem_ctx in this function? What about using db (or a special memctx
instead of db) instead, then the caller could just talloc_free_children
(or we can talloc_free_children even in this function..)

>  +
>  +    free(line);
>  +    line = NULL; 
>  From 0877b501f01dff26a312944a47a98cb88eb47d1e Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  Date: Wed, 19 Aug 2015 12:43:15 +0200
>  Subject: [PATCH 3/5] sss_override: decompose code better 
ACK

thanks for splitting the patch, makes the review much easier.

>  From 1556d7f744302e8e7e3aa4e3459c5a4c7467bbd9 Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  Date: Wed, 19 Aug 2015 12:35:12 +0200
>  Subject: [PATCH 4/5] sss_override: support import and export
>  
>  Resolves:
>  https://fedorahosted.org/sssd/ticket/2737 
Please ask Dan to add the import and export to his tests.

Please also ask him to test UPN in the first column -- that's what's the
Dell tool supports.

I'm having trouble overriding GIDs, do these work for you? For users,
all attributes work fine for me, also when I run "sss_override group-add".

My importfile looked like this:
# cat importgroup 
[e-mail]:sssdgroup123:
[e-mail]:sssdgroup124:124


>  ---
>   Makefile.am                |   2 +
>   src/man/sss_override.8.xml |  88 +++++++
>   src/tools/sss_override.c   | 596 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 686 insertions(+)
>  
>  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/man/sss_override.8.xml b/src/man/sss_override.8.xml
>  index ec9a7bb75c13f4f18ece7f5f84baede14a8a1e2e..edb77d43f54b3f70d8fdb7260c4c8bc9e6c225d8
> 100644
>  --- a/src/man/sss_override.8.xml
>  +++ b/src/man/sss_override.8.xml
>  @@ -77,6 +77,50 @@
>               </varlistentry>
>               <varlistentry>
>                   <term>
>  +                    <option>user-import</option>
>  +                    <emphasis>FILE</emphasis>
>  +                </term>
>  +                <listitem>
>  +                    <para>
>  +                        Import user overrides from
> <emphasis>FILE</emphasis>.
>  +                        Data format is similar to standard passwd file.
>  +                        The format is: 
>  +                    </para>
>  +                    <para>
>  +                        original_name:name:uid:gid:gecos:home:shell
>  +                    </para>
>  +                    <para>
>  +                        where original_name is original name of the user whose
>  +                        attributes should be overridden. The rest of fields
>  +                        correspond to new values. You can omit a value simply
>  +                        by leaving corresponding field empty.
>  +                    </para>
>  +                    <para>
>  +                        Examples:
>  +                    </para>
>  +                    <para>
>  +                        ckent:superman::::::
>  +                    </para>
>  +                    <para>
>  +                        [e-mail]::501:501:/home/earth:/bin/bash:Superman
Unless having Superman as the shell is part of super-powers, I suspect
the order of the fields in the example doesn't match :)

>  +                    </para>
>  +                </listitem>
>  +            </varlistentry>
>  +            <varlistentry>
>  +                <term>
>  +                    <option>user-export</option>
>  +                    <emphasis>FILE</emphasis>
>  +                </term>
>  +                <listitem>
>  +                    <para>
>  +                        Export all overridden attributes and store them in
>  +                        <emphasis>FILE</emphasis>. See
>  +                        <emphasis>user-import</emphasis> for data format. 
>  +                    </para>
>  +                </listitem>
>  +            </varlistentry>
>  +            <varlistentry>
>  +                <term>
>                       <option>group-add</option>
>                       <emphasis>NAME</emphasis>
>                       <optional><option>-n,--name</option>
> NAME</optional>
>  @@ -99,6 +143,50 @@
>                       </para>
>                   </listitem>
>               </varlistentry>
>  +            <varlistentry>
>  +                <term>
>  +                    <option>group-import</option>
>  +                    <emphasis>FILE</emphasis>
>  +                </term>
>  +                <listitem>
>  +                    <para>
>  +                        Import group overrides from
> <emphasis>FILE</emphasis>.
>  +                        Data format is similar to standard group file.
>  +                        The format is: 
>  +                    </para>
>  +                    <para>
>  +                        original_name:name:gid
>  +                    </para>
>  +                    <para>
>  +                        where original_name is original name of the group whose
>  +                        attributes should be overridden. The rest of fields
>  +                        correspond to new values. You can omit a value simply
>  +                        by leaving corresponding field empty.
>  +                    </para>
>  +                    <para>
>  +                        Examples:
>  +                    </para>
>  +                    <para>
>  +                        admins:administrators: 
Isn't there supposed to be an empty field?
    admins:administrators::

>  +                    </para>
>  +                    <para>
>  +                        Domain Users:Users:501
>  +                    </para>
>  +                </listitem>
>  +            </varlistentry>
>  +            <varlistentry>
>  +                <term>
>  +                    <option>group-export</option>
>  +                    <emphasis>FILE</emphasis>
>  +                </term>
>  +                <listitem>
>  +                    <para>
>  +                        Export all overridden attributes and store them in
>  +                        <emphasis>FILE</emphasis>. See
>  +                        <emphasis>group-import</emphasis> for data format. 
>  +                    </para>
>  +                </listitem>
>  +            </varlistentry>
>           </variablelist>
>       </refsect1> 
[...]

>  +static int override_user_import(struct sss_cmdline *cmdline,
>  +                                struct sss_tool_ctx *tool_ctx,
>  +                                void *pvt)
>  +{
>  +    TALLOC_CTX *tmp_ctx;
>  +    struct sss_colondb *db;
>  +    const char *filename;
>  +    struct override_user obj;
>  +    int linenum = 1;
>  +    errno_t ret;
>  +    int exit;
>  +
>  +    tmp_ctx = talloc_new(NULL);
>  +    if (tmp_ctx == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n");
>  +        return EXIT_FAILURE;
>  +    }
>  +
>  +    /**
>  +     * Format: orig_name:name:uid:gid:gecos:home:shell
>  +     */
>  +    struct sss_colondb_read_field table[] = {
>  +        {SSS_COLONDB_STRING, {.str = &obj.input_name}},
>  +        {SSS_COLONDB_STRING, {.str = &obj.name}},
>  +        {SSS_COLONDB_UINT32, {.uint32 = &obj.uid}},
>  +        {SSS_COLONDB_UINT32, {.uint32 = &obj.gid}},
>  +        {SSS_COLONDB_STRING, {.str = &obj.gecos}},
>  +        {SSS_COLONDB_STRING, {.str = &obj.home}},
>  +        {SSS_COLONDB_STRING, {.str = &obj.shell}},
>  +        {SSS_COLONDB_SENTINEL, {0}}
>  +    };
>  +
>  +    ret = parse_cmdline_import(cmdline, tool_ctx, &filename);
>  +    if (ret != EOK) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    db = sss_colondb_open(tool_ctx, SSS_COLONDB_READ, filename);
>  +    if (db == NULL) {
>  +        fprintf(stderr, _("Unable to open %s.\n"), filename);
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +
>  +    tmp_ctx = talloc_new(NULL); 
You assigned twice to tmp_ctx in this function. Also in
override_group_import.

>  +    if (tmp_ctx == NULL) {
>  +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
>  +        exit = EXIT_FAILURE;
>  +        goto done;
>  +    }
>  +

[Pavel Březina]

On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
>  On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
> > On 08/18/2015 05:24 PM, Pavel Březina wrote:
> >> On 08/16/2015 05:59 PM, 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.
> >>
> >> New patches attached.
> >
> > I split it into more patches.
> >
>
>  There is a strange ordering between this patch and the FQDN -- these
>  patches can't be compiled without the FQDN one, but the FQDN doesn't
>  apply on master. 
Thanks, I fixed that. I'm sending all the patches here including the fqn 
one in correct order.

>  There are still some Coverity errors:
>  Error: COMPILER_WARNING:
>  sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
> 'sss_colondb_open'
>  sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may be used
> uninitialized in this function [-Wmaybe-uninitialized]
>  #     if (fp == NULL && filename != NULL) {
>  #            ^
>  sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was declared here
>  #     FILE *fp;
>  #           ^
>  #  271|       }
>  #  272|
>  #  273|->     if (fp == NULL && filename != NULL) {
>  #  274|           ret = errno;
>  #  275|           DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s [%d]:
> %s\n",
>
>  Error: NEGATIVE_RETURNS (CWE-394):
>  sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
> "sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a negative
> number.
>  sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant: Explicitly returning
> negative value "-22".
>  sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning: unsigned variable
> "fqlen" = "sss_fqname".
>  sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned variable
> "fqlen" is incremented, which might cause an integer overflow.
>  #  340|       fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
>  #  341|       if (fqlen > 0) {
>  #  342|->         fqlen++; /* \0 */
>  #  343|       } else {
>  #  344|           return NULL; 
Hopefully fixed now.

> >  From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> > Date: Sat, 15 Aug 2015 13:53:25 +0200
> > Subject: [PATCH 1/5] sss_override: print input name if unable to parse it
>
>  ACK
>
>
> >  From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> > Date: Wed, 19 Aug 2015 12:34:08 +0200
> > Subject: [PATCH 2/5] TOOLS: add sss_colondb API
> >
> > To simplify import/export users and groups.
>
>  More or less ack :-) see some questions inline
>
> > +errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
> > +                             struct sss_colondb *db,
> > +                             struct sss_colondb_read_field *table)
> > +{
> > +    int readchars;
> > +    size_t linelen = 0;
> > +    char *line = NULL;
> > +    char *tcline;
> > +    char *rest;
> > +    errno_t ret;
> > +    int i;
> > +
> > +    if (db->mode != SSS_COLONDB_READ) {
> > +        return ERR_INTERNAL;
> > +    }
> > +
> > +    readchars = getline(&line, &linelen, db->file);
> > +    if (readchars == -1) {
> > +        /* Nothing was read. */
> > +        if (errno != 0) {
> > +            ret = errno;
> > +            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]: %s",
> > +                  ret, sss_strerror(ret));
> > +            return ret;
> > +        }
> > +
> > +        return EOF;
> > +    }
> > +
> > +    /* Copy line to mem_ctx. */
> > +    tcline = talloc_strdup(mem_ctx, line);
>
>  Do we need mem_ctx in this function? What about using db (or a special memctx
>  instead of db) instead, then the caller could just talloc_free_children
>  (or we can talloc_free_children even in this function..) 
Yes, it is needed in my opinion. Let us see if we both understand the 
purpose here, if not I'll add a comment.

We read line with getline() which returns a pointer that needs to be 
freed. But we read data into read_field table and then use it in the 
caller. The problem we need to solve is how to push line pointer to the 
caller so it can be freed when data is no longer needed. Is it clear?

I can think of three solutions:
1) Pass *line as output parameter so the caller can free it.
2) Use talloc and talloc_strdup all strings to mem_ctx.
3) Use talloc but strdup the whole line to mem_ctx.

I chose 3) since it contains less allocations and it is quite simple and 
elegant.

You can't free data in sss_colondb_readline() because you don't know 
when the caller stops using them. And calling talloc_free_children on 
encapsulated db context is not safe. Yes, it is doable now since it 
doesn't contain any children, but the context structure can be changed 
any time in the future.

Maybe I don't understand what you mean by "special memctx" since that is 
what I use there.

> > +
> > +    free(line);
> > +    line = NULL;
>
> >  From 0877b501f01dff26a312944a47a98cb88eb47d1e Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> > Date: Wed, 19 Aug 2015 12:43:15 +0200
> > Subject: [PATCH 3/5] sss_override: decompose code better
>
>  ACK
>
>  thanks for splitting the patch, makes the review much easier.
>
> >  From 1556d7f744302e8e7e3aa4e3459c5a4c7467bbd9 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> > Date: Wed, 19 Aug 2015 12:35:12 +0200
> > Subject: [PATCH 4/5] sss_override: support import and export
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2737
>
>  Please ask Dan to add the import and export to his tests.
>
>  Please also ask him to test UPN in the first column -- that's what's the
>  Dell tool supports. 
UPNs won't work yet, I think. I need to finally recreate my AD 
environment before I start working on those.

>  I'm having trouble overriding GIDs, do these work for you? For
> users,
>  all attributes work fine for me, also when I run "sss_override group-add".
>
>  My importfile looked like this:
>  # cat importgroup
>  [e-mail]:sssdgroup123:
>  [e-mail]:sssdgroup124:124
>
>
> > ---
> >   Makefile.am                |   2 +
> >   src/man/sss_override.8.xml |  88 +++++++
> >   src/tools/sss_override.c   | 596 +++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 686 insertions(+)
> >
> > 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/man/sss_override.8.xml b/src/man/sss_override.8.xml
> > index
> ec9a7bb75c13f4f18ece7f5f84baede14a8a1e2e..edb77d43f54b3f70d8fdb7260c4c8bc9e6c225d8 100644
> > --- a/src/man/sss_override.8.xml
> > +++ b/src/man/sss_override.8.xml
> > @@ -77,6 +77,50 @@
> >               </varlistentry>
> >               <varlistentry>
> >                   <term>
> > +                    <option>user-import</option>
> > +                    <emphasis>FILE</emphasis>
> > +                </term>
> > +                <listitem>
> > +                    <para>
> > +                        Import user overrides from
> <emphasis>FILE</emphasis>.
> > +                        Data format is similar to standard passwd file.
> > +                        The format is:
> > +                    </para>
> > +                    <para>
> > +                        original_name:name:uid:gid:gecos:home:shell
> > +                    </para>
> > +                    <para>
> > +                        where original_name is original name of the user whose
> > +                        attributes should be overridden. The rest of fields
> > +                        correspond to new values. You can omit a value simply
> > +                        by leaving corresponding field empty.
> > +                    </para>
> > +                    <para>
> > +                        Examples:
> > +                    </para>
> > +                    <para>
> > +                        ckent:superman::::::
> > +                    </para>
> > +                    <para>
> > +                        [e-mail]::501:501:/home/earth:/bin/bash:Superman
>
>  Unless having Superman as the shell is part of super-powers, I suspect
>  the order of the fields in the example doesn't match :)
>
> > +                    </para>
> > +                </listitem>
> > +            </varlistentry>
> > +            <varlistentry>
> > +                <term>
> > +                    <option>user-export</option>
> > +                    <emphasis>FILE</emphasis>
> > +                </term>
> > +                <listitem>
> > +                    <para>
> > +                        Export all overridden attributes and store them in
> > +                        <emphasis>FILE</emphasis>. See
> > +                        <emphasis>user-import</emphasis> for data
> format.
> > +                    </para>
> > +                </listitem>
> > +            </varlistentry>
> > +            <varlistentry>
> > +                <term>
> >                       <option>group-add</option>
> >                       <emphasis>NAME</emphasis>
> >                       <optional><option>-n,--name</option>
> NAME</optional>
> > @@ -99,6 +143,50 @@
> >                       </para>
> >                   </listitem>
> >               </varlistentry>
> > +            <varlistentry>
> > +                <term>
> > +                    <option>group-import</option>
> > +                    <emphasis>FILE</emphasis>
> > +                </term>
> > +                <listitem>
> > +                    <para>
> > +                        Import group overrides from
> <emphasis>FILE</emphasis>.
> > +                        Data format is similar to standard group file.
> > +                        The format is:
> > +                    </para>
> > +                    <para>
> > +                        original_name:name:gid
> > +                    </para>
> > +                    <para>
> > +                        where original_name is original name of the group whose
> > +                        attributes should be overridden. The rest of fields
> > +                        correspond to new values. You can omit a value simply
> > +                        by leaving corresponding field empty.
> > +                    </para>
> > +                    <para>
> > +                        Examples:
> > +                    </para>
> > +                    <para>
> > +                        admins:administrators:
>
>  Isn't there supposed to be an empty field?
>       admins:administrators::
>
> > +                    </para>
> > +                    <para>
> > +                        Domain Users:Users:501
> > +                    </para>
> > +                </listitem>
> > +            </varlistentry>
> > +            <varlistentry>
> > +                <term>
> > +                    <option>group-export</option>
> > +                    <emphasis>FILE</emphasis>
> > +                </term>
> > +                <listitem>
> > +                    <para>
> > +                        Export all overridden attributes and store them in
> > +                        <emphasis>FILE</emphasis>. See
> > +                        <emphasis>group-import</emphasis> for data
> format.
> > +                    </para>
> > +                </listitem>
> > +            </varlistentry>
> >           </variablelist>
> >       </refsect1>
>
>  [...]
>
> > +static int override_user_import(struct sss_cmdline *cmdline,
> > +                                struct sss_tool_ctx *tool_ctx,
> > +                                void *pvt)
> > +{
> > +    TALLOC_CTX *tmp_ctx;
> > +    struct sss_colondb *db;
> > +    const char *filename;
> > +    struct override_user obj;
> > +    int linenum = 1;
> > +    errno_t ret;
> > +    int exit;
> > +
> > +    tmp_ctx = talloc_new(NULL);
> > +    if (tmp_ctx == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n");
> > +        return EXIT_FAILURE;
> > +    }
> > +
> > +    /**
> > +     * Format: orig_name:name:uid:gid:gecos:home:shell
> > +     */
> > +    struct sss_colondb_read_field table[] = {
> > +        {SSS_COLONDB_STRING, {.str = &obj.input_name}},
> > +        {SSS_COLONDB_STRING, {.str = &obj.name}},
> > +        {SSS_COLONDB_UINT32, {.uint32 = &obj.uid}},
> > +        {SSS_COLONDB_UINT32, {.uint32 = &obj.gid}},
> > +        {SSS_COLONDB_STRING, {.str = &obj.gecos}},
> > +        {SSS_COLONDB_STRING, {.str = &obj.home}},
> > +        {SSS_COLONDB_STRING, {.str = &obj.shell}},
> > +        {SSS_COLONDB_SENTINEL, {0}}
> > +    };
> > +
> > +    ret = parse_cmdline_import(cmdline, tool_ctx, &filename);
> > +    if (ret != EOK) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");
> > +        exit = EXIT_FAILURE;
> > +        goto done;
> > +    }
> > +
> > +    db = sss_colondb_open(tool_ctx, SSS_COLONDB_READ, filename);
> > +    if (db == NULL) {
> > +        fprintf(stderr, _("Unable to open %s.\n"), filename);
> > +        exit = EXIT_FAILURE;
> > +        goto done;
> > +    }
> > +
> > +    tmp_ctx = talloc_new(NULL);
>
>  You assigned twice to tmp_ctx in this function. Also in
>  override_group_import.
>
> > +    if (tmp_ctx == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> > +        exit = EXIT_FAILURE;
> > +        goto done;
> > +    }
> > +
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

[Jakub Hrozek]

On Thu, Aug 20, 2015 at 12:54:47PM +0200, Pavel Březina wrote:
>  On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
>  >On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
>  >>On 08/18/2015 05:24 PM, Pavel Březina wrote:
>  >>>On 08/16/2015 05:59 PM, 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.
>  >>>
>  >>>New patches attached.
>  >>
>  >>I split it into more patches.
>  >>
>  >
>  >There is a strange ordering between this patch and the FQDN -- these
>  >patches can't be compiled without the FQDN one, but the FQDN doesn't
>  >apply on master.
>  
>  Thanks, I fixed that. I'm sending all the patches here including the fqn one
>  in correct order.
>  
>  >There are still some Coverity errors:
>  >Error: COMPILER_WARNING:
>  >sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
> 'sss_colondb_open'
>  >sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may be used
> uninitialized in this function [-Wmaybe-uninitialized]
>  >#     if (fp == NULL && filename != NULL) {
>  >#            ^
>  >sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was declared
> here
>  >#     FILE *fp;
>  >#           ^
>  >#  271|       }
>  >#  272|
>  >#  273|->     if (fp == NULL && filename != NULL) {
>  >#  274|           ret = errno;
>  >#  275|           DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s [%d]:
> %s\n",
>  >
>  >Error: NEGATIVE_RETURNS (CWE-394):
>  >sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
> "sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a negative
> number.
>  >sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant: Explicitly
> returning negative value "-22".
>  >sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning: unsigned variable
> "fqlen" = "sss_fqname".
>  >sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned variable
> "fqlen" is incremented, which might cause an integer overflow.
>  >#  340|       fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
>  >#  341|       if (fqlen > 0) {
>  >#  342|->         fqlen++; /* \0 */
>  >#  343|       } else {
>  >#  344|           return NULL;
>  
>  Hopefully fixed now.
>  
>  >
>  >> From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
>  >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  >>Date: Sat, 15 Aug 2015 13:53:25 +0200
>  >>Subject: [PATCH 1/5] sss_override: print input name if unable to parse it
>  >
>  >ACK
>  >
>  >
>  >> From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
>  >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  >>Date: Wed, 19 Aug 2015 12:34:08 +0200
>  >>Subject: [PATCH 2/5] TOOLS: add sss_colondb API
>  >>
>  >>To simplify import/export users and groups.
>  >
>  >More or less ack :-) see some questions inline
>  >
>  >>+errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
>  >>+                             struct sss_colondb *db,
>  >>+                             struct sss_colondb_read_field *table)
>  >>+{
>  >>+    int readchars;
>  >>+    size_t linelen = 0;
>  >>+    char *line = NULL;
>  >>+    char *tcline;
>  >>+    char *rest;
>  >>+    errno_t ret;
>  >>+    int i;
>  >>+
>  >>+    if (db->mode != SSS_COLONDB_READ) {
>  >>+        return ERR_INTERNAL;
>  >>+    }
>  >>+
>  >>+    readchars = getline(&line, &linelen, db->file);
>  >>+    if (readchars == -1) {
>  >>+        /* Nothing was read. */
>  >>+        if (errno != 0) {
>  >>+            ret = errno;
>  >>+            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]:
> %s",
>  >>+                  ret, sss_strerror(ret));
>  >>+            return ret;
>  >>+        }
>  >>+
>  >>+        return EOF;
>  >>+    }
>  >>+
>  >>+    /* Copy line to mem_ctx. */
>  >>+    tcline = talloc_strdup(mem_ctx, line);
>  >
>  >Do we need mem_ctx in this function? What about using db (or a special memctx
>  >instead of db) instead, then the caller could just talloc_free_children
>  >(or we can talloc_free_children even in this function..)
>  
>  Yes, it is needed in my opinion. Let us see if we both understand the
>  purpose here, if not I'll add a comment.
>  
>  We read line with getline() which returns a pointer that needs to be freed.
>  But we read data into read_field table and then use it in the caller. The
>  problem we need to solve is how to push line pointer to the caller so it can
>  be freed when data is no longer needed. Is it clear?
>  
>  I can think of three solutions:
>  1) Pass *line as output parameter so the caller can free it.
>  2) Use talloc and talloc_strdup all strings to mem_ctx.
>  3) Use talloc but strdup the whole line to mem_ctx.
>  
>  I chose 3) since it contains less allocations and it is quite simple and
>  elegant.
>  
>  You can't free data in sss_colondb_readline() because you don't know when
>  the caller stops using them. And calling talloc_free_children on
>  encapsulated db context is not safe. Yes, it is doable now since it doesn't
>  contain any children, but the context structure can be changed any time in
>  the future. 
I would argue that when the caller calls sss_colondb_readline he's done
using the data from the previous line.

>  
>  Maybe I don't understand what you mean by "special memctx" since that is
>  what I use there. 
I meant to have something like:
    sss_colondb_ctx -+
                     +-- readline_ctx

And allocate the line contents internally on readline_ctx.

But I'm not sure if an internal interface is worth all the hassle..

>  
>  >
>  >>+
>  >>+    free(line);
>  >>+    line = NULL;
>  >
>  >> From 0877b501f01dff26a312944a47a98cb88eb47d1e Mon Sep 17 00:00:00 2001
>  >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  >>Date: Wed, 19 Aug 2015 12:43:15 +0200
>  >>Subject: [PATCH 3/5] sss_override: decompose code better
>  >
>  >ACK
>  >
>  >thanks for splitting the patch, makes the review much easier.
>  >
>  >> From 1556d7f744302e8e7e3aa4e3459c5a4c7467bbd9 Mon Sep 17 00:00:00 2001
>  >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  >>Date: Wed, 19 Aug 2015 12:35:12 +0200
>  >>Subject: [PATCH 4/5] sss_override: support import and export
>  >>
>  >>Resolves:
>  >>https://fedorahosted.org/sssd/ticket/2737
>  >
>  >Please ask Dan to add the import and export to his tests.
>  >
>  >Please also ask him to test UPN in the first column -- that's what's the
>  >Dell tool supports.
>  
>  UPNs won't work yet, I think. I need to finally recreate my AD environment
>  before I start working on those. 
Please do, do you want me to file a ticket so the effort is tracked? I
haven't tested this scenario either, but the NSS responder supports
lookups by UPNs for some time now...

I'll take a look at the patches..

[Pavel Březina]

On 08/20/2015 01:31 PM, Jakub Hrozek wrote:
>  On Thu, Aug 20, 2015 at 12:54:47PM +0200, Pavel Březina wrote:
> > On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
> >> On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
> >>> On 08/18/2015 05:24 PM, Pavel Březina wrote:
> >>>> On 08/16/2015 05:59 PM, 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.
> >>>>
> >>>> New patches attached.
> >>>
> >>> I split it into more patches.
> >>>
> >>
> >> There is a strange ordering between this patch and the FQDN -- these
> >> patches can't be compiled without the FQDN one, but the FQDN doesn't
> >> apply on master.
> >
> > Thanks, I fixed that. I'm sending all the patches here including the fqn one
> > in correct order.
> >
> >> There are still some Coverity errors:
> >> Error: COMPILER_WARNING:
> >> sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
> 'sss_colondb_open'
> >> sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may be
> used uninitialized in this function [-Wmaybe-uninitialized]
> >> #     if (fp == NULL && filename != NULL) {
> >> #            ^
> >> sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was
> declared here
> >> #     FILE *fp;
> >> #           ^
> >> #  271|       }
> >> #  272|
> >> #  273|->     if (fp == NULL && filename != NULL) {
> >> #  274|           ret = errno;
> >> #  275|           DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s [%d]:
> %s\n",
> >>
> >> Error: NEGATIVE_RETURNS (CWE-394):
> >> sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
> "sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a negative
> number.
> >> sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant: Explicitly
> returning negative value "-22".
> >> sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning: unsigned
> variable "fqlen" = "sss_fqname".
> >> sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned variable
> "fqlen" is incremented, which might cause an integer overflow.
> >> #  340|       fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
> >> #  341|       if (fqlen > 0) {
> >> #  342|->         fqlen++; /* \0 */
> >> #  343|       } else {
> >> #  344|           return NULL;
> >
> > Hopefully fixed now.
> >
> >>
> >>>  From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
> >>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> >>> Date: Sat, 15 Aug 2015 13:53:25 +0200
> >>> Subject: [PATCH 1/5] sss_override: print input name if unable to parse it
> >>
> >> ACK
> >>
> >>
> >>>  From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
> >>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> >>> Date: Wed, 19 Aug 2015 12:34:08 +0200
> >>> Subject: [PATCH 2/5] TOOLS: add sss_colondb API
> >>>
> >>> To simplify import/export users and groups.
> >>
> >> More or less ack :-) see some questions inline
> >>
> >>> +errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
> >>> +                             struct sss_colondb *db,
> >>> +                             struct sss_colondb_read_field *table)
> >>> +{
> >>> +    int readchars;
> >>> +    size_t linelen = 0;
> >>> +    char *line = NULL;
> >>> +    char *tcline;
> >>> +    char *rest;
> >>> +    errno_t ret;
> >>> +    int i;
> >>> +
> >>> +    if (db->mode != SSS_COLONDB_READ) {
> >>> +        return ERR_INTERNAL;
> >>> +    }
> >>> +
> >>> +    readchars = getline(&line, &linelen, db->file);
> >>> +    if (readchars == -1) {
> >>> +        /* Nothing was read. */
> >>> +        if (errno != 0) {
> >>> +            ret = errno;
> >>> +            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]:
> %s",
> >>> +                  ret, sss_strerror(ret));
> >>> +            return ret;
> >>> +        }
> >>> +
> >>> +        return EOF;
> >>> +    }
> >>> +
> >>> +    /* Copy line to mem_ctx. */
> >>> +    tcline = talloc_strdup(mem_ctx, line);
> >>
> >> Do we need mem_ctx in this function? What about using db (or a special memctx
> >> instead of db) instead, then the caller could just talloc_free_children
> >> (or we can talloc_free_children even in this function..)
> >
> > Yes, it is needed in my opinion. Let us see if we both understand the
> > purpose here, if not I'll add a comment.
> >
> > We read line with getline() which returns a pointer that needs to be freed.
> > But we read data into read_field table and then use it in the caller. The
> > problem we need to solve is how to push line pointer to the caller so it can
> > be freed when data is no longer needed. Is it clear?
> >
> > I can think of three solutions:
> > 1) Pass *line as output parameter so the caller can free it.
> > 2) Use talloc and talloc_strdup all strings to mem_ctx.
> > 3) Use talloc but strdup the whole line to mem_ctx.
> >
> > I chose 3) since it contains less allocations and it is quite simple and
> > elegant.
> >
> > You can't free data in sss_colondb_readline() because you don't know when
> > the caller stops using them. And calling talloc_free_children on
> > encapsulated db context is not safe. Yes, it is doable now since it doesn't
> > contain any children, but the context structure can be changed any time in
> > the future.
>
>  I would argue that when the caller calls sss_colondb_readline he's done
>  using the data from the previous line. 
In this use case, yes. This way it is more generic though.

> >
> > Maybe I don't understand what you mean by "special memctx" since that
> is
> > what I use there.
>
>  I meant to have something like:
>       sss_colondb_ctx -+
>                        +-- readline_ctx
>
>  And allocate the line contents internally on readline_ctx.
>
>  But I'm not sure if an internal interface is worth all the hassle.. 
Well, I stand by current code but I don't have any valid arguments to 
oppose you since this is doable as well :-) You're the one who's going 
to ack it, so it's your call.

> >
> >>
> >>> +
> >>> +    free(line);
> >>> +    line = NULL;
> >>
> >>>  From 0877b501f01dff26a312944a47a98cb88eb47d1e Mon Sep 17 00:00:00 2001
> >>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> >>> Date: Wed, 19 Aug 2015 12:43:15 +0200
> >>> Subject: [PATCH 3/5] sss_override: decompose code better
> >>
> >> ACK
> >>
> >> thanks for splitting the patch, makes the review much easier.
> >>
> >>>  From 1556d7f744302e8e7e3aa4e3459c5a4c7467bbd9 Mon Sep 17 00:00:00 2001
> >>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> >>> Date: Wed, 19 Aug 2015 12:35:12 +0200
> >>> Subject: [PATCH 4/5] sss_override: support import and export
> >>>
> >>> Resolves:
> >>> https://fedorahosted.org/sssd/ticket/2737
> >>
> >> Please ask Dan to add the import and export to his tests.
> >>
> >> Please also ask him to test UPN in the first column -- that's what's the
> >> Dell tool supports.
> >
> > UPNs won't work yet, I think. I need to finally recreate my AD environment
> > before I start working on those.
>
>  Please do, do you want me to file a ticket so the effort is tracked? I
>  haven't tested this scenario either, but the NSS responder supports
>  lookups by UPNs for some time now... 
Yes please.

>  I'll take a look at the patches..

[Pavel Březina]

On 08/20/2015 12:54 PM, Pavel Březina wrote:
>  On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
> > On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
> >> On 08/18/2015 05:24 PM, Pavel Březina wrote:
> >>> On 08/16/2015 05:59 PM, 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.
> >>>
> >>> New patches attached.
> >>
> >> I split it into more patches.
> >>
> >
> > There is a strange ordering between this patch and the FQDN -- these
> > patches can't be compiled without the FQDN one, but the FQDN doesn't
> > apply on master.
>
>  Thanks, I fixed that. I'm sending all the patches here including the fqn
>  one in correct order.
>
> > There are still some Coverity errors:
> > Error: COMPILER_WARNING:
> > sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
> > 'sss_colondb_open'
> > sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may
> > be used uninitialized in this function [-Wmaybe-uninitialized]
> > #     if (fp == NULL && filename != NULL) {
> > #            ^
> > sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was
> > declared here
> > #     FILE *fp;
> > #           ^
> > #  271|       }
> > #  272|
> > #  273|->     if (fp == NULL && filename != NULL) {
> > #  274|           ret = errno;
> > #  275|           DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s
> > [%d]: %s\n",
> >
> > Error: NEGATIVE_RETURNS (CWE-394):
> > sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
> > "sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a
> > negative number.
> > sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant:
> > Explicitly returning negative value "-22".
> > sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning:
> > unsigned variable "fqlen" = "sss_fqname".
> > sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned
> > variable "fqlen" is incremented, which might cause an integer overflow.
> > #  340|       fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
> > #  341|       if (fqlen > 0) {
> > #  342|->         fqlen++; /* \0 */
> > #  343|       } else {
> > #  344|           return NULL;
>
>  Hopefully fixed now.
>
> >
> >>  From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
> >> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> >> Date: Sat, 15 Aug 2015 13:53:25 +0200
> >> Subject: [PATCH 1/5] sss_override: print input name if unable to
> >> parse it
> >
> > ACK
> >
> >
> >>  From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
> >> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> >> Date: Wed, 19 Aug 2015 12:34:08 +0200
> >> Subject: [PATCH 2/5] TOOLS: add sss_colondb API
> >>
> >> To simplify import/export users and groups.
> >
> > More or less ack :-) see some questions inline
> >
> >> +errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
> >> +                             struct sss_colondb *db,
> >> +                             struct sss_colondb_read_field *table)
> >> +{
> >> +    int readchars;
> >> +    size_t linelen = 0;
> >> +    char *line = NULL;
> >> +    char *tcline;
> >> +    char *rest;
> >> +    errno_t ret;
> >> +    int i;
> >> +
> >> +    if (db->mode != SSS_COLONDB_READ) {
> >> +        return ERR_INTERNAL;
> >> +    }
> >> +
> >> +    readchars = getline(&line, &linelen, db->file);
> >> +    if (readchars == -1) {
> >> +        /* Nothing was read. */
> >> +        if (errno != 0) {
> >> +            ret = errno;
> >> +            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]:
> %s",
> >> +                  ret, sss_strerror(ret));
> >> +            return ret;
> >> +        }
> >> +
> >> +        return EOF;
> >> +    }
> >> +
> >> +    /* Copy line to mem_ctx. */
> >> +    tcline = talloc_strdup(mem_ctx, line);
> >
> > Do we need mem_ctx in this function? What about using db (or a special
> > memctx
> > instead of db) instead, then the caller could just talloc_free_children
> > (or we can talloc_free_children even in this function..)
>
>  Yes, it is needed in my opinion. Let us see if we both understand the
>  purpose here, if not I'll add a comment.
>
>  We read line with getline() which returns a pointer that needs to be
>  freed. But we read data into read_field table and then use it in the
>  caller. The problem we need to solve is how to push line pointer to the
>  caller so it can be freed when data is no longer needed. Is it clear?
>
>  I can think of three solutions:
>  1) Pass *line as output parameter so the caller can free it.
>  2) Use talloc and talloc_strdup all strings to mem_ctx.
>  3) Use talloc but strdup the whole line to mem_ctx.
>
>  I chose 3) since it contains less allocations and it is quite simple and
>  elegant.
>
>  You can't free data in sss_colondb_readline() because you don't know
>  when the caller stops using them. And calling talloc_free_children on
>  encapsulated db context is not safe. Yes, it is doable now since it
>  doesn't contain any children, but the context structure can be changed
>  any time in the future.
>
>  Maybe I don't understand what you mean by "special memctx" since that is
>  what I use there.
>
> >
> >> +
> >> +    free(line);
> >> +    line = NULL;
> >
> >>  From 0877b501f01dff26a312944a47a98cb88eb47d1e Mon Sep 17 00:00:00 2001
> >> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> >> Date: Wed, 19 Aug 2015 12:43:15 +0200
> >> Subject: [PATCH 3/5] sss_override: decompose code better
> >
> > ACK
> >
> > thanks for splitting the patch, makes the review much easier.
> >
> >>  From 1556d7f744302e8e7e3aa4e3459c5a4c7467bbd9 Mon Sep 17 00:00:00 2001
> >> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> >> Date: Wed, 19 Aug 2015 12:35:12 +0200
> >> Subject: [PATCH 4/5] sss_override: support import and export
> >>
> >> Resolves:
> >> https://fedorahosted.org/sssd/ticket/2737
> >
> > Please ask Dan to add the import and export to his tests.
> >
> > Please also ask him to test UPN in the first column -- that's what's the
> > Dell tool supports.
>
>  UPNs won't work yet, I think. I need to finally recreate my AD
>  environment before I start working on those.
>
> > I'm having trouble overriding GIDs, do these work for you? For users,
> > all attributes work fine for me, also when I run "sss_override
> > group-add".
> >
> > My importfile looked like this:
> > # cat importgroup
> > [e-mail]:sssdgroup123:
> > [e-mail]:sssdgroup124:124
> >
> >
> >> ---
> >>   Makefile.am                |   2 +
> >>   src/man/sss_override.8.xml |  88 +++++++
> >>   src/tools/sss_override.c   | 596
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >>   3 files changed, 686 insertions(+)
> >>
> >> 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/man/sss_override.8.xml b/src/man/sss_override.8.xml
> >> index
> >>
> ec9a7bb75c13f4f18ece7f5f84baede14a8a1e2e..edb77d43f54b3f70d8fdb7260c4c8bc9e6c225d8
> >> 100644
> >> --- a/src/man/sss_override.8.xml
> >> +++ b/src/man/sss_override.8.xml
> >> @@ -77,6 +77,50 @@
> >>               </varlistentry>
> >>               <varlistentry>
> >>                   <term>
> >> +                    <option>user-import</option>
> >> +                    <emphasis>FILE</emphasis>
> >> +                </term>
> >> +                <listitem>
> >> +                    <para>
> >> +                        Import user overrides from
> >> <emphasis>FILE</emphasis>.
> >> +                        Data format is similar to standard passwd file.
> >> +                        The format is:
> >> +                    </para>
> >> +                    <para>
> >> +                        original_name:name:uid:gid:gecos:home:shell
> >> +                    </para>
> >> +                    <para>
> >> +                        where original_name is original name of the
> >> user whose
> >> +                        attributes should be overridden. The rest of
> >> fields
> >> +                        correspond to new values. You can omit a
> >> value simply
> >> +                        by leaving corresponding field empty.
> >> +                    </para>
> >> +                    <para>
> >> +                        Examples:
> >> +                    </para>
> >> +                    <para>
> >> +                        ckent:superman::::::
> >> +                    </para>
> >> +                    <para>
> >> +
> >> [e-mail]::501:501:/home/earth:/bin/bash:Superman
> >
> > Unless having Superman as the shell is part of super-powers, I suspect
> > the order of the fields in the example doesn't match :) 
Thanks :-)

> >
> >> +                    </para>
> >> +                </listitem>
> >> +            </varlistentry>
> >> +            <varlistentry>
> >> +                <term>
> >> +                    <option>user-export</option>
> >> +                    <emphasis>FILE</emphasis>
> >> +                </term>
> >> +                <listitem>
> >> +                    <para>
> >> +                        Export all overridden attributes and store
> >> them in
> >> +                        <emphasis>FILE</emphasis>. See
> >> +                        <emphasis>user-import</emphasis> for data
> >> format.
> >> +                    </para>
> >> +                </listitem>
> >> +            </varlistentry>
> >> +            <varlistentry>
> >> +                <term>
> >>                       <option>group-add</option>
> >>                       <emphasis>NAME</emphasis>
> >>                       <optional><option>-n,--name</option>
> >> NAME</optional>
> >> @@ -99,6 +143,50 @@
> >>                       </para>
> >>                   </listitem>
> >>               </varlistentry>
> >> +            <varlistentry>
> >> +                <term>
> >> +                    <option>group-import</option>
> >> +                    <emphasis>FILE</emphasis>
> >> +                </term>
> >> +                <listitem>
> >> +                    <para>
> >> +                        Import group overrides from
> >> <emphasis>FILE</emphasis>.
> >> +                        Data format is similar to standard group file.
> >> +                        The format is:
> >> +                    </para>
> >> +                    <para>
> >> +                        original_name:name:gid
> >> +                    </para>
> >> +                    <para>
> >> +                        where original_name is original name of the
> >> group whose
> >> +                        attributes should be overridden. The rest of
> >> fields
> >> +                        correspond to new values. You can omit a
> >> value simply
> >> +                        by leaving corresponding field empty.
> >> +                    </para>
> >> +                    <para>
> >> +                        Examples:
> >> +                    </para>
> >> +                    <para>
> >> +                        admins:administrators:
> >
> > Isn't there supposed to be an empty field?
> >      admins:administrators:: 
No, the format is:

orig_name:new_name:new_gid

therefore there is no colon at the end.

> >
> >> +                    </para>
> >> +                    <para>
> >> +                        Domain Users:Users:501
> >> +                    </para>
> >> +                </listitem>
> >> +            </varlistentry>
> >> +            <varlistentry>
> >> +                <term>
> >> +                    <option>group-export</option>
> >> +                    <emphasis>FILE</emphasis>
> >> +                </term>
> >> +                <listitem>
> >> +                    <para>
> >> +                        Export all overridden attributes and store
> >> them in
> >> +                        <emphasis>FILE</emphasis>. See
> >> +                        <emphasis>group-import</emphasis> for data
> >> format.
> >> +                    </para>
> >> +                </listitem>
> >> +            </varlistentry>
> >>           </variablelist>
> >>       </refsect1>
> >
> > [...]
> >
> >> +static int override_user_import(struct sss_cmdline *cmdline,
> >> +                                struct sss_tool_ctx *tool_ctx,
> >> +                                void *pvt)
> >> +{
> >> +    TALLOC_CTX *tmp_ctx;
> >> +    struct sss_colondb *db;
> >> +    const char *filename;
> >> +    struct override_user obj;
> >> +    int linenum = 1;
> >> +    errno_t ret;
> >> +    int exit;
> >> +
> >> +    tmp_ctx = talloc_new(NULL);
> >> +    if (tmp_ctx == NULL) {
> >> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n");
> >> +        return EXIT_FAILURE;
> >> +    }
> >> +
> >> +    /**
> >> +     * Format: orig_name:name:uid:gid:gecos:home:shell
> >> +     */
> >> +    struct sss_colondb_read_field table[] = {
> >> +        {SSS_COLONDB_STRING, {.str = &obj.input_name}},
> >> +        {SSS_COLONDB_STRING, {.str = &obj.name}},
> >> +        {SSS_COLONDB_UINT32, {.uint32 = &obj.uid}},
> >> +        {SSS_COLONDB_UINT32, {.uint32 = &obj.gid}},
> >> +        {SSS_COLONDB_STRING, {.str = &obj.gecos}},
> >> +        {SSS_COLONDB_STRING, {.str = &obj.home}},
> >> +        {SSS_COLONDB_STRING, {.str = &obj.shell}},
> >> +        {SSS_COLONDB_SENTINEL, {0}}
> >> +    };
> >> +
> >> +    ret = parse_cmdline_import(cmdline, tool_ctx, &filename);
> >> +    if (ret != EOK) {
> >> +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command
> line.\n");
> >> +        exit = EXIT_FAILURE;
> >> +        goto done;
> >> +    }
> >> +
> >> +    db = sss_colondb_open(tool_ctx, SSS_COLONDB_READ, filename);
> >> +    if (db == NULL) {
> >> +        fprintf(stderr, _("Unable to open %s.\n"), filename);
> >> +        exit = EXIT_FAILURE;
> >> +        goto done;
> >> +    }
> >> +
> >> +    tmp_ctx = talloc_new(NULL);
> >
> > You assigned twice to tmp_ctx in this function. Also in
> > override_group_import. 
Fixed.

Here are patches with rest of comments addressed.

[Jakub Hrozek]

On Thu, Aug 20, 2015 at 01:55:45PM +0200, Pavel Březina wrote:
>  On 08/20/2015 12:54 PM, Pavel Březina wrote:
>  >On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
>  >>On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
>  >>>On 08/18/2015 05:24 PM, Pavel Březina wrote:
>  >>>>On 08/16/2015 05:59 PM, 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.
>  >>>>
>  >>>>New patches attached.
>  >>>
>  >>>I split it into more patches.
>  >>>
>  >>
>  >>There is a strange ordering between this patch and the FQDN -- these
>  >>patches can't be compiled without the FQDN one, but the FQDN doesn't
>  >>apply on master.
>  >
>  >Thanks, I fixed that. I'm sending all the patches here including the fqn
>  >one in correct order.
>  >
>  >>There are still some Coverity errors:
>  >>Error: COMPILER_WARNING:
>  >>sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
>  >>'sss_colondb_open'
>  >>sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may
>  >>be used uninitialized in this function [-Wmaybe-uninitialized]
>  >>#     if (fp == NULL && filename != NULL) {
>  >>#            ^
>  >>sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was
>  >>declared here
>  >>#     FILE *fp;
>  >>#           ^
>  >>#  271|       }
>  >>#  272|
>  >>#  273|->     if (fp == NULL && filename != NULL) {
>  >>#  274|           ret = errno;
>  >>#  275|           DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s
>  >>[%d]: %s\n",
>  >>
>  >>Error: NEGATIVE_RETURNS (CWE-394):
>  >>sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
>  >>"sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a
>  >>negative number.
>  >>sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant:
>  >>Explicitly returning negative value "-22".
>  >>sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning:
>  >>unsigned variable "fqlen" = "sss_fqname".
>  >>sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned
>  >>variable "fqlen" is incremented, which might cause an integer
> overflow.
>  >>#  340|       fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
>  >>#  341|       if (fqlen > 0) {
>  >>#  342|->         fqlen++; /* \0 */
>  >>#  343|       } else {
>  >>#  344|           return NULL;
>  >
>  >Hopefully fixed now.
>  >
>  >>
>  >>> From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
>  >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  >>>Date: Sat, 15 Aug 2015 13:53:25 +0200
>  >>>Subject: [PATCH 1/5] sss_override: print input name if unable to
>  >>>parse it
>  >>
>  >>ACK
>  >>
>  >>
>  >>> From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
>  >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  >>>Date: Wed, 19 Aug 2015 12:34:08 +0200
>  >>>Subject: [PATCH 2/5] TOOLS: add sss_colondb API
>  >>>
>  >>>To simplify import/export users and groups.
>  >>
>  >>More or less ack :-) see some questions inline
>  >>
>  >>>+errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
>  >>>+                             struct sss_colondb *db,
>  >>>+                             struct sss_colondb_read_field *table)
>  >>>+{
>  >>>+    int readchars;
>  >>>+    size_t linelen = 0;
>  >>>+    char *line = NULL;
>  >>>+    char *tcline;
>  >>>+    char *rest;
>  >>>+    errno_t ret;
>  >>>+    int i;
>  >>>+
>  >>>+    if (db->mode != SSS_COLONDB_READ) {
>  >>>+        return ERR_INTERNAL;
>  >>>+    }
>  >>>+
>  >>>+    readchars = getline(&line, &linelen, db->file);
>  >>>+    if (readchars == -1) {
>  >>>+        /* Nothing was read. */
>  >>>+        if (errno != 0) {
>  >>>+            ret = errno;
>  >>>+            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]:
> %s",
>  >>>+                  ret, sss_strerror(ret));
>  >>>+            return ret;
>  >>>+        }
>  >>>+
>  >>>+        return EOF;
>  >>>+    }
>  >>>+
>  >>>+    /* Copy line to mem_ctx. */
>  >>>+    tcline = talloc_strdup(mem_ctx, line);
>  >>
>  >>Do we need mem_ctx in this function? What about using db (or a special
>  >>memctx
>  >>instead of db) instead, then the caller could just talloc_free_children
>  >>(or we can talloc_free_children even in this function..)
>  >
>  >Yes, it is needed in my opinion. Let us see if we both understand the
>  >purpose here, if not I'll add a comment.
>  >
>  >We read line with getline() which returns a pointer that needs to be
>  >freed. But we read data into read_field table and then use it in the
>  >caller. The problem we need to solve is how to push line pointer to the
>  >caller so it can be freed when data is no longer needed. Is it clear?
>  >
>  >I can think of three solutions:
>  >1) Pass *line as output parameter so the caller can free it.
>  >2) Use talloc and talloc_strdup all strings to mem_ctx.
>  >3) Use talloc but strdup the whole line to mem_ctx.
>  >
>  >I chose 3) since it contains less allocations and it is quite simple and
>  >elegant.
>  >
>  >You can't free data in sss_colondb_readline() because you don't know
>  >when the caller stops using them. And calling talloc_free_children on
>  >encapsulated db context is not safe. Yes, it is doable now since it
>  >doesn't contain any children, but the context structure can be changed
>  >any time in the future.
>  >
>  >Maybe I don't understand what you mean by "special memctx" since that
> is
>  >what I use there.
>  >
>  >>
>  >>>+
>  >>>+    free(line);
>  >>>+    line = NULL;
>  >>
>  >>> From 0877b501f01dff26a312944a47a98cb88eb47d1e Mon Sep 17 00:00:00 2001
>  >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  >>>Date: Wed, 19 Aug 2015 12:43:15 +0200
>  >>>Subject: [PATCH 3/5] sss_override: decompose code better
>  >>
>  >>ACK
>  >>
>  >>thanks for splitting the patch, makes the review much easier.
>  >>
>  >>> From 1556d7f744302e8e7e3aa4e3459c5a4c7467bbd9 Mon Sep 17 00:00:00 2001
>  >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  >>>Date: Wed, 19 Aug 2015 12:35:12 +0200
>  >>>Subject: [PATCH 4/5] sss_override: support import and export
>  >>>
>  >>>Resolves:
>  >>>https://fedorahosted.org/sssd/ticket/2737
>  >>
>  >>Please ask Dan to add the import and export to his tests.
>  >>
>  >>Please also ask him to test UPN in the first column -- that's what's the
>  >>Dell tool supports.
>  >
>  >UPNs won't work yet, I think. I need to finally recreate my AD
>  >environment before I start working on those.
>  >
>  >>I'm having trouble overriding GIDs, do these work for you? For users,
>  >>all attributes work fine for me, also when I run "sss_override
>  >>group-add".
>  >>
>  >>My importfile looked like this:
>  >># cat importgroup
>  >>[e-mail]:sssdgroup123:
>  >>[e-mail]:sssdgroup124:124
>  >>
>  >>
>  >>>---
>  >>>  Makefile.am                |   2 +
>  >>>  src/man/sss_override.8.xml |  88 +++++++
>  >>>  src/tools/sss_override.c   | 596
>  >>>+++++++++++++++++++++++++++++++++++++++++++++
>  >>>  3 files changed, 686 insertions(+)
>  >>>
>  >>>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/man/sss_override.8.xml b/src/man/sss_override.8.xml
>  >>>index
>
> >>>ec9a7bb75c13f4f18ece7f5f84baede14a8a1e2e..edb77d43f54b3f70d8fdb7260c4c8bc9e6c225d8
>  >>>100644
>  >>>--- a/src/man/sss_override.8.xml
>  >>>+++ b/src/man/sss_override.8.xml
>  >>>@@ -77,6 +77,50 @@
>  >>>              </varlistentry>
>  >>>              <varlistentry>
>  >>>                  <term>
>  >>>+                    <option>user-import</option>
>  >>>+                    <emphasis>FILE</emphasis>
>  >>>+                </term>
>  >>>+                <listitem>
>  >>>+                    <para>
>  >>>+                        Import user overrides from
>  >>><emphasis>FILE</emphasis>.
>  >>>+                        Data format is similar to standard passwd file.
>  >>>+                        The format is:
>  >>>+                    </para>
>  >>>+                    <para>
>  >>>+                        original_name:name:uid:gid:gecos:home:shell
>  >>>+                    </para>
>  >>>+                    <para>
>  >>>+                        where original_name is original name of the
>  >>>user whose
>  >>>+                        attributes should be overridden. The rest of
>  >>>fields
>  >>>+                        correspond to new values. You can omit a
>  >>>value simply
>  >>>+                        by leaving corresponding field empty.
>  >>>+                    </para>
>  >>>+                    <para>
>  >>>+                        Examples:
>  >>>+                    </para>
>  >>>+                    <para>
>  >>>+                        ckent:superman::::::
>  >>>+                    </para>
>  >>>+                    <para>
>  >>>+
>  >>>[e-mail]::501:501:/home/earth:/bin/bash:Superman
>  >>
>  >>Unless having Superman as the shell is part of super-powers, I suspect
>  >>the order of the fields in the example doesn't match :)
>  
>  Thanks :-)
>  
>  >>
>  >>>+                    </para>
>  >>>+                </listitem>
>  >>>+            </varlistentry>
>  >>>+            <varlistentry>
>  >>>+                <term>
>  >>>+                    <option>user-export</option>
>  >>>+                    <emphasis>FILE</emphasis>
>  >>>+                </term>
>  >>>+                <listitem>
>  >>>+                    <para>
>  >>>+                        Export all overridden attributes and store
>  >>>them in
>  >>>+                        <emphasis>FILE</emphasis>. See
>  >>>+                        <emphasis>user-import</emphasis> for
> data
>  >>>format.
>  >>>+                    </para>
>  >>>+                </listitem>
>  >>>+            </varlistentry>
>  >>>+            <varlistentry>
>  >>>+                <term>
>  >>>                      <option>group-add</option>
>  >>>                      <emphasis>NAME</emphasis>
>  >>>                      <optional><option>-n,--name</option>
>  >>>NAME</optional>
>  >>>@@ -99,6 +143,50 @@
>  >>>                      </para>
>  >>>                  </listitem>
>  >>>              </varlistentry>
>  >>>+            <varlistentry>
>  >>>+                <term>
>  >>>+                    <option>group-import</option>
>  >>>+                    <emphasis>FILE</emphasis>
>  >>>+                </term>
>  >>>+                <listitem>
>  >>>+                    <para>
>  >>>+                        Import group overrides from
>  >>><emphasis>FILE</emphasis>.
>  >>>+                        Data format is similar to standard group file.
>  >>>+                        The format is:
>  >>>+                    </para>
>  >>>+                    <para>
>  >>>+                        original_name:name:gid
>  >>>+                    </para>
>  >>>+                    <para>
>  >>>+                        where original_name is original name of the
>  >>>group whose
>  >>>+                        attributes should be overridden. The rest of
>  >>>fields
>  >>>+                        correspond to new values. You can omit a
>  >>>value simply
>  >>>+                        by leaving corresponding field empty.
>  >>>+                    </para>
>  >>>+                    <para>
>  >>>+                        Examples:
>  >>>+                    </para>
>  >>>+                    <para>
>  >>>+                        admins:administrators:
>  >>
>  >>Isn't there supposed to be an empty field?
>  >>     admins:administrators::
>  
>  No, the format is:
>  
>  orig_name:new_name:new_gid
>  
>  therefore there is no colon at the end.
>  
>  >>
>  >>>+                    </para>
>  >>>+                    <para>
>  >>>+                        Domain Users:Users:501
>  >>>+                    </para>
>  >>>+                </listitem>
>  >>>+            </varlistentry>
>  >>>+            <varlistentry>
>  >>>+                <term>
>  >>>+                    <option>group-export</option>
>  >>>+                    <emphasis>FILE</emphasis>
>  >>>+                </term>
>  >>>+                <listitem>
>  >>>+                    <para>
>  >>>+                        Export all overridden attributes and store
>  >>>them in
>  >>>+                        <emphasis>FILE</emphasis>. See
>  >>>+                        <emphasis>group-import</emphasis> for
> data
>  >>>format.
>  >>>+                    </para>
>  >>>+                </listitem>
>  >>>+            </varlistentry>
>  >>>          </variablelist>
>  >>>      </refsect1>
>  >>
>  >>[...]
>  >>
>  >>>+static int override_user_import(struct sss_cmdline *cmdline,
>  >>>+                                struct sss_tool_ctx *tool_ctx,
>  >>>+                                void *pvt)
>  >>>+{
>  >>>+    TALLOC_CTX *tmp_ctx;
>  >>>+    struct sss_colondb *db;
>  >>>+    const char *filename;
>  >>>+    struct override_user obj;
>  >>>+    int linenum = 1;
>  >>>+    errno_t ret;
>  >>>+    int exit;
>  >>>+
>  >>>+    tmp_ctx = talloc_new(NULL);
>  >>>+    if (tmp_ctx == NULL) {
>  >>>+        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n");
>  >>>+        return EXIT_FAILURE;
>  >>>+    }
>  >>>+
>  >>>+    /**
>  >>>+     * Format: orig_name:name:uid:gid:gecos:home:shell
>  >>>+     */
>  >>>+    struct sss_colondb_read_field table[] = {
>  >>>+        {SSS_COLONDB_STRING, {.str = &obj.input_name}},
>  >>>+        {SSS_COLONDB_STRING, {.str = &obj.name}},
>  >>>+        {SSS_COLONDB_UINT32, {.uint32 = &obj.uid}},
>  >>>+        {SSS_COLONDB_UINT32, {.uint32 = &obj.gid}},
>  >>>+        {SSS_COLONDB_STRING, {.str = &obj.gecos}},
>  >>>+        {SSS_COLONDB_STRING, {.str = &obj.home}},
>  >>>+        {SSS_COLONDB_STRING, {.str = &obj.shell}},
>  >>>+        {SSS_COLONDB_SENTINEL, {0}}
>  >>>+    };
>  >>>+
>  >>>+    ret = parse_cmdline_import(cmdline, tool_ctx, &filename);
>  >>>+    if (ret != EOK) {
>  >>>+        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command
> line.\n");
>  >>>+        exit = EXIT_FAILURE;
>  >>>+        goto done;
>  >>>+    }
>  >>>+
>  >>>+    db = sss_colondb_open(tool_ctx, SSS_COLONDB_READ, filename);
>  >>>+    if (db == NULL) {
>  >>>+        fprintf(stderr, _("Unable to open %s.\n"), filename);
>  >>>+        exit = EXIT_FAILURE;
>  >>>+        goto done;
>  >>>+    }
>  >>>+
>  >>>+    tmp_ctx = talloc_new(NULL);
>  >>
>  >>You assigned twice to tmp_ctx in this function. Also in
>  >>override_group_import.
>  
>  Fixed.
>  
>  Here are patches with rest of comments addressed. 
Thank you.

I tried a couple of more tests like overriding UIDs or GIDs to zero and
everything worked fine.

Coverity is clean, CI as well.

ACK

Thank you for the hard work on these patches!

[Jakub Hrozek]

On Thu, Aug 20, 2015 at 10:39:06PM +0200, Jakub Hrozek wrote:
>  On Thu, Aug 20, 2015 at 01:55:45PM +0200, Pavel Březina wrote:
>  > On 08/20/2015 12:54 PM, Pavel Březina wrote:
>  > >On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
>  > >>On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
>  > >>>On 08/18/2015 05:24 PM, Pavel Březina wrote:
>  > >>>>On 08/16/2015 05:59 PM, 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.
>  > >>>>
>  > >>>>New patches attached.
>  > >>>
>  > >>>I split it into more patches.
>  > >>>
>  > >>
>  > >>There is a strange ordering between this patch and the FQDN -- these
>  > >>patches can't be compiled without the FQDN one, but the FQDN
> doesn't
>  > >>apply on master.
>  > >
>  > >Thanks, I fixed that. I'm sending all the patches here including the fqn
>  > >one in correct order.
>  > >
>  > >>There are still some Coverity errors:
>  > >>Error: COMPILER_WARNING:
>  > >>sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
>  > >>'sss_colondb_open'
>  > >>sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp'
> may
>  > >>be used uninitialized in this function [-Wmaybe-uninitialized]
>  > >>#     if (fp == NULL && filename != NULL) {
>  > >>#            ^
>  > >>sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was
>  > >>declared here
>  > >>#     FILE *fp;
>  > >>#           ^
>  > >>#  271|       }
>  > >>#  272|
>  > >>#  273|->     if (fp == NULL && filename != NULL) {
>  > >>#  274|           ret = errno;
>  > >>#  275|           DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s
>  > >>[%d]: %s\n",
>  > >>
>  > >>Error: NEGATIVE_RETURNS (CWE-394):
>  > >>sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
>  > >>"sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a
>  > >>negative number.
>  > >>sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant:
>  > >>Explicitly returning negative value "-22".
>  > >>sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning:
>  > >>unsigned variable "fqlen" = "sss_fqname".
>  > >>sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned
>  > >>variable "fqlen" is incremented, which might cause an integer
> overflow.
>  > >>#  340|       fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
>  > >>#  341|       if (fqlen > 0) {
>  > >>#  342|->         fqlen++; /* \0 */
>  > >>#  343|       } else {
>  > >>#  344|           return NULL;
>  > >
>  > >Hopefully fixed now.
>  > >
>  > >>
>  > >>> From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
>  > >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  > >>>Date: Sat, 15 Aug 2015 13:53:25 +0200
>  > >>>Subject: [PATCH 1/5] sss_override: print input name if unable to
>  > >>>parse it
>  > >>
>  > >>ACK
>  > >>
>  > >>
>  > >>> From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
>  > >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  > >>>Date: Wed, 19 Aug 2015 12:34:08 +0200
>  > >>>Subject: [PATCH 2/5] TOOLS: add sss_colondb API
>  > >>>
>  > >>>To simplify import/export users and groups.
>  > >>
>  > >>More or less ack :-) see some questions inline
>  > >>
>  > >>>+errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
>  > >>>+                             struct sss_colondb *db,
>  > >>>+                             struct sss_colondb_read_field *table)
>  > >>>+{
>  > >>>+    int readchars;
>  > >>>+    size_t linelen = 0;
>  > >>>+    char *line = NULL;
>  > >>>+    char *tcline;
>  > >>>+    char *rest;
>  > >>>+    errno_t ret;
>  > >>>+    int i;
>  > >>>+
>  > >>>+    if (db->mode != SSS_COLONDB_READ) {
>  > >>>+        return ERR_INTERNAL;
>  > >>>+    }
>  > >>>+
>  > >>>+    readchars = getline(&line, &linelen, db->file);
>  > >>>+    if (readchars == -1) {
>  > >>>+        /* Nothing was read. */
>  > >>>+        if (errno != 0) {
>  > >>>+            ret = errno;
>  > >>>+            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]:
> %s",
>  > >>>+                  ret, sss_strerror(ret));
>  > >>>+            return ret;
>  > >>>+        }
>  > >>>+
>  > >>>+        return EOF;
>  > >>>+    }
>  > >>>+
>  > >>>+    /* Copy line to mem_ctx. */
>  > >>>+    tcline = talloc_strdup(mem_ctx, line);
>  > >>
>  > >>Do we need mem_ctx in this function? What about using db (or a special
>  > >>memctx
>  > >>instead of db) instead, then the caller could just talloc_free_children
>  > >>(or we can talloc_free_children even in this function..)
>  > >
>  > >Yes, it is needed in my opinion. Let us see if we both understand the
>  > >purpose here, if not I'll add a comment.
>  > >
>  > >We read line with getline() which returns a pointer that needs to be
>  > >freed. But we read data into read_field table and then use it in the
>  > >caller. The problem we need to solve is how to push line pointer to the
>  > >caller so it can be freed when data is no longer needed. Is it clear?
>  > >
>  > >I can think of three solutions:
>  > >1) Pass *line as output parameter so the caller can free it.
>  > >2) Use talloc and talloc_strdup all strings to mem_ctx.
>  > >3) Use talloc but strdup the whole line to mem_ctx.
>  > >
>  > >I chose 3) since it contains less allocations and it is quite simple and
>  > >elegant.
>  > >
>  > >You can't free data in sss_colondb_readline() because you don't know
>  > >when the caller stops using them. And calling talloc_free_children on
>  > >encapsulated db context is not safe. Yes, it is doable now since it
>  > >doesn't contain any children, but the context structure can be changed
>  > >any time in the future.
>  > >
>  > >Maybe I don't understand what you mean by "special memctx" since
> that is
>  > >what I use there.
>  > >
>  > >>
>  > >>>+
>  > >>>+    free(line);
>  > >>>+    line = NULL;
>  > >>
>  > >>> From 0877b501f01dff26a312944a47a98cb88eb47d1e Mon Sep 17 00:00:00 2001
>  > >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  > >>>Date: Wed, 19 Aug 2015 12:43:15 +0200
>  > >>>Subject: [PATCH 3/5] sss_override: decompose code better
>  > >>
>  > >>ACK
>  > >>
>  > >>thanks for splitting the patch, makes the review much easier.
>  > >>
>  > >>> From 1556d7f744302e8e7e3aa4e3459c5a4c7467bbd9 Mon Sep 17 00:00:00 2001
>  > >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  > >>>Date: Wed, 19 Aug 2015 12:35:12 +0200
>  > >>>Subject: [PATCH 4/5] sss_override: support import and export
>  > >>>
>  > >>>Resolves:
>  > >>>https://fedorahosted.org/sssd/ticket/2737
>  > >>
>  > >>Please ask Dan to add the import and export to his tests.
>  > >>
>  > >>Please also ask him to test UPN in the first column -- that's what's
> the
>  > >>Dell tool supports.
>  > >
>  > >UPNs won't work yet, I think. I need to finally recreate my AD
>  > >environment before I start working on those.
>  > >
>  > >>I'm having trouble overriding GIDs, do these work for you? For users,
>  > >>all attributes work fine for me, also when I run "sss_override
>  > >>group-add".
>  > >>
>  > >>My importfile looked like this:
>  > >># cat importgroup
>  > >>[e-mail]:sssdgroup123:
>  > >>[e-mail]:sssdgroup124:124
>  > >>
>  > >>
>  > >>>---
>  > >>>  Makefile.am                |   2 +
>  > >>>  src/man/sss_override.8.xml |  88 +++++++
>  > >>>  src/tools/sss_override.c   | 596
>  > >>>+++++++++++++++++++++++++++++++++++++++++++++
>  > >>>  3 files changed, 686 insertions(+)
>  > >>>
>  > >>>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/man/sss_override.8.xml b/src/man/sss_override.8.xml
>  > >>>index
>  >
> >>>ec9a7bb75c13f4f18ece7f5f84baede14a8a1e2e..edb77d43f54b3f70d8fdb7260c4c8bc9e6c225d8
>  > >>>100644
>  > >>>--- a/src/man/sss_override.8.xml
>  > >>>+++ b/src/man/sss_override.8.xml
>  > >>>@@ -77,6 +77,50 @@
>  > >>>              </varlistentry>
>  > >>>              <varlistentry>
>  > >>>                  <term>
>  > >>>+                    <option>user-import</option>
>  > >>>+                    <emphasis>FILE</emphasis>
>  > >>>+                </term>
>  > >>>+                <listitem>
>  > >>>+                    <para>
>  > >>>+                        Import user overrides from
>  > >>><emphasis>FILE</emphasis>.
>  > >>>+                        Data format is similar to standard passwd
> file.
>  > >>>+                        The format is:
>  > >>>+                    </para>
>  > >>>+                    <para>
>  > >>>+                        original_name:name:uid:gid:gecos:home:shell
>  > >>>+                    </para>
>  > >>>+                    <para>
>  > >>>+                        where original_name is original name of the
>  > >>>user whose
>  > >>>+                        attributes should be overridden. The rest of
>  > >>>fields
>  > >>>+                        correspond to new values. You can omit a
>  > >>>value simply
>  > >>>+                        by leaving corresponding field empty.
>  > >>>+                    </para>
>  > >>>+                    <para>
>  > >>>+                        Examples:
>  > >>>+                    </para>
>  > >>>+                    <para>
>  > >>>+                        ckent:superman::::::
>  > >>>+                    </para>
>  > >>>+                    <para>
>  > >>>+
>  > >>>[e-mail]::501:501:/home/earth:/bin/bash:Superman
>  > >>
>  > >>Unless having Superman as the shell is part of super-powers, I suspect
>  > >>the order of the fields in the example doesn't match :)
>  > 
>  > Thanks :-)
>  > 
>  > >>
>  > >>>+                    </para>
>  > >>>+                </listitem>
>  > >>>+            </varlistentry>
>  > >>>+            <varlistentry>
>  > >>>+                <term>
>  > >>>+                    <option>user-export</option>
>  > >>>+                    <emphasis>FILE</emphasis>
>  > >>>+                </term>
>  > >>>+                <listitem>
>  > >>>+                    <para>
>  > >>>+                        Export all overridden attributes and store
>  > >>>them in
>  > >>>+                        <emphasis>FILE</emphasis>. See
>  > >>>+                        <emphasis>user-import</emphasis>
> for data
>  > >>>format.
>  > >>>+                    </para>
>  > >>>+                </listitem>
>  > >>>+            </varlistentry>
>  > >>>+            <varlistentry>
>  > >>>+                <term>
>  > >>>                      <option>group-add</option>
>  > >>>                      <emphasis>NAME</emphasis>
>  > >>>                     
> <optional><option>-n,--name</option>
>  > >>>NAME</optional>
>  > >>>@@ -99,6 +143,50 @@
>  > >>>                      </para>
>  > >>>                  </listitem>
>  > >>>              </varlistentry>
>  > >>>+            <varlistentry>
>  > >>>+                <term>
>  > >>>+                    <option>group-import</option>
>  > >>>+                    <emphasis>FILE</emphasis>
>  > >>>+                </term>
>  > >>>+                <listitem>
>  > >>>+                    <para>
>  > >>>+                        Import group overrides from
>  > >>><emphasis>FILE</emphasis>.
>  > >>>+                        Data format is similar to standard group file.
>  > >>>+                        The format is:
>  > >>>+                    </para>
>  > >>>+                    <para>
>  > >>>+                        original_name:name:gid
>  > >>>+                    </para>
>  > >>>+                    <para>
>  > >>>+                        where original_name is original name of the
>  > >>>group whose
>  > >>>+                        attributes should be overridden. The rest of
>  > >>>fields
>  > >>>+                        correspond to new values. You can omit a
>  > >>>value simply
>  > >>>+                        by leaving corresponding field empty.
>  > >>>+                    </para>
>  > >>>+                    <para>
>  > >>>+                        Examples:
>  > >>>+                    </para>
>  > >>>+                    <para>
>  > >>>+                        admins:administrators:
>  > >>
>  > >>Isn't there supposed to be an empty field?
>  > >>     admins:administrators::
>  > 
>  > No, the format is:
>  > 
>  > orig_name:new_name:new_gid
>  > 
>  > therefore there is no colon at the end.
>  > 
>  > >>
>  > >>>+                    </para>
>  > >>>+                    <para>
>  > >>>+                        Domain Users:Users:501
>  > >>>+                    </para>
>  > >>>+                </listitem>
>  > >>>+            </varlistentry>
>  > >>>+            <varlistentry>
>  > >>>+                <term>
>  > >>>+                    <option>group-export</option>
>  > >>>+                    <emphasis>FILE</emphasis>
>  > >>>+                </term>
>  > >>>+                <listitem>
>  > >>>+                    <para>
>  > >>>+                        Export all overridden attributes and store
>  > >>>them in
>  > >>>+                        <emphasis>FILE</emphasis>. See
>  > >>>+                        <emphasis>group-import</emphasis>
> for data
>  > >>>format.
>  > >>>+                    </para>
>  > >>>+                </listitem>
>  > >>>+            </varlistentry>
>  > >>>          </variablelist>
>  > >>>      </refsect1>
>  > >>
>  > >>[...]
>  > >>
>  > >>>+static int override_user_import(struct sss_cmdline *cmdline,
>  > >>>+                                struct sss_tool_ctx *tool_ctx,
>  > >>>+                                void *pvt)
>  > >>>+{
>  > >>>+    TALLOC_CTX *tmp_ctx;
>  > >>>+    struct sss_colondb *db;
>  > >>>+    const char *filename;
>  > >>>+    struct override_user obj;
>  > >>>+    int linenum = 1;
>  > >>>+    errno_t ret;
>  > >>>+    int exit;
>  > >>>+
>  > >>>+    tmp_ctx = talloc_new(NULL);
>  > >>>+    if (tmp_ctx == NULL) {
>  > >>>+        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new()
> failed.\n");
>  > >>>+        return EXIT_FAILURE;
>  > >>>+    }
>  > >>>+
>  > >>>+    /**
>  > >>>+     * Format: orig_name:name:uid:gid:gecos:home:shell
>  > >>>+     */
>  > >>>+    struct sss_colondb_read_field table[] = {
>  > >>>+        {SSS_COLONDB_STRING, {.str = &obj.input_name}},
>  > >>>+        {SSS_COLONDB_STRING, {.str = &obj.name}},
>  > >>>+        {SSS_COLONDB_UINT32, {.uint32 = &obj.uid}},
>  > >>>+        {SSS_COLONDB_UINT32, {.uint32 = &obj.gid}},
>  > >>>+        {SSS_COLONDB_STRING, {.str = &obj.gecos}},
>  > >>>+        {SSS_COLONDB_STRING, {.str = &obj.home}},
>  > >>>+        {SSS_COLONDB_STRING, {.str = &obj.shell}},
>  > >>>+        {SSS_COLONDB_SENTINEL, {0}}
>  > >>>+    };
>  > >>>+
>  > >>>+    ret = parse_cmdline_import(cmdline, tool_ctx, &filename);
>  > >>>+    if (ret != EOK) {
>  > >>>+        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command
> line.\n");
>  > >>>+        exit = EXIT_FAILURE;
>  > >>>+        goto done;
>  > >>>+    }
>  > >>>+
>  > >>>+    db = sss_colondb_open(tool_ctx, SSS_COLONDB_READ, filename);
>  > >>>+    if (db == NULL) {
>  > >>>+        fprintf(stderr, _("Unable to open %s.\n"),
> filename);
>  > >>>+        exit = EXIT_FAILURE;
>  > >>>+        goto done;
>  > >>>+    }
>  > >>>+
>  > >>>+    tmp_ctx = talloc_new(NULL);
>  > >>
>  > >>You assigned twice to tmp_ctx in this function. Also in
>  > >>override_group_import.
>  > 
>  > Fixed.
>  > 
>  > Here are patches with rest of comments addressed.
>  
>  Thank you.
>  
>  I tried a couple of more tests like overriding UIDs or GIDs to zero and
>  everything worked fine.
>  
>  Coverity is clean, CI as well.
>  
>  ACK
>  
>  Thank you for the hard work on these patches! 
Pushed to master:
    * 23fb01bf67a6058fb508da6d81515e8b18634beb
    * 5df5a6b852eccaafc8a3fb4eb31296d9587be483
    * a76f63544533f0404f7711a10c1a621c6045df17
    * 7eba58cfcf78e61af1c4ff98619aa97223eb7a5b
    * 4285cf181abd1d12dc144d5f86d73162bbd9cf05