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>
>> 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>
>> +
>> + 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++;