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>
>>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>
>>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>
>>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>
>>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..