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>
>>> 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.
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>
>>> 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...
Yes please.
I'll take a look at the patches..