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

Pavel Březina pbrezina at redhat.com
Thu Aug 20 11:47:55 UTC 2015


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




More information about the sssd-devel mailing list