[SSSD] [PATCHES] Sysdb interface for netgroups

Dmitri Pal dpal at redhat.com
Mon Sep 20 11:44:02 UTC 2010


Jan Zelený wrote:
> Stephen Gallagher <sgallagh at redhat.com> wrote:
>   
>> On 09/17/2010 07:18 PM, Jan Zeleny wrote:
>>     
>>> I went through the code of the first patch and there are some things
>>> which don't seem to be right.
>>>
>>> First of all, the code is missing the memory hierarchy entirely. Here are
>>> some examples:
>>> src/db/sys_ops.c:
>>> 1125, 1175, 2504, 2567 - parent context should be taken as the first
>>> argument 1132, 1186, 2517, 2578 - parent context should be here instead
>>> of NULL
>>>       
>> No, this is intentional. There are several places in the sysdb where
>> we're doing this wrong, because once upon a time the sysdb was an
>> asynchronous interface. When this was the case, it was necessary for
>> these functions to have a memory context, because we needed to be able
>> to cancel them if the parent context was canceled.
>>
>> Nowadays, the proper behavior is that a synchronous function should NOT
>> take a talloc context unless it is returning data. Internally to the
>> synchronous function, this should be the ONLY memory allocated atop this
>> context. Any other memory allocated during the synchronous run should be
>> allocated atop a tmp_ctx that is a child of NULL.
>>
>> The reason for this is that it makes it much easier to track down memory
>> leaks. Valgrind is unable to detect when memory is still in use if it's
>> been attached to another memory context (because it's still referenced
>> by a variable that is in scope).
>>
>> If the memory context happens to be something long-lived (for example
>> it's a direct child of the responder context or nss context) then this
>> memory may leak on every invocation without a way to see that it happened.
>>
>> However, if the temporary context for this function is allocated on
>> NULL, when the function exits, if tmp_ctx hasn't been fully destroyed,
>> there will remain a detectable memory leak that we can use to identify
>> the source of growth.
>>
>> In conclusion: this is a feature, not a bug :)
>>     
>
> Thanks for the explanation. It certainly sounds reasonable. And if there is a 
> policy, that synchronous functions don't need to have memory hierarchy, there 
> is no need to discuss this further.
>
> Just one thing. For the sake of code lucidity, I'd prefer if this was unified 
> through the code.
>   


Me too. Please open a ticket.

>   
>>> Regarding errno_t: you use it in several functions. That's probably
>>> right, but I think if we want to take this approach, we should update
>>> all functions to follow rule. For example function sysdb_add_netgroup
>>> should use it for returning errno as well.
>>>       
>> We decided a long time ago that we would start using errno_t when it was
>> appropriate in order to more clearly differentiate what the return codes
>> are. Since we already had so much code in place, however, we just agreed
>> that we would only worry about declaring new functions that way (since
>> it would be a time-consuming effort for no tangible benefit to change
>> the other functions).
>>
>> On all platforms (by standard) errno_t == int.
>>     
>
> Ok. But I still think it would be better to update the code so errno_t is used 
> where possible. This way it is too confusing, at least for me.
>
>   
>>> Then I suggest a minor improvement:
>>> src/db/sys_ops.c:1230 - the DEBUG macro will be better if called just
>>> before the talloc_zfree on 1233 (in case committing the transaction
>>> failed)
>>>       
>> I don't understand what you want me to do here. The DEBUG statement here
>> is meant to return the reason why the operation failed (and as a result,
>> why the transaction was to be canceled).
>>
>> On the other hand, I did notice that it might be wiser to change this to:
>>
>>     if (ret == EOK) {
>>         ret = ldb_transaction_commit(ctx->ldb);
>>         ret = sysdb_error_to_errno(ret);
>>     }
>>
>>     if (ret != EOK) {
>>         DEBUG(6, ("Error: %d (%s)\n", ret, strerror(ret)));
>>         ldb_transaction_cancel(ctx->ldb);
>>     }
>>
>> So that if a failure occurred in the transaction commit, we would then
>> call transaction cancel.
>>     
>
> That's exactly what I meant. Thanks
>
> I have last two things. I talked with Jakub and we discussed those functions 
> sysdb_add_netgroup_tuple, etc. Are they really necessary? They just call other 
> functions with predefined parameter, therefore I'd say they are just aliases, 
> which aren't really needed.
>
> And the last thing: I was wondering about those FIXMEs - are they resolved 
> yet? If yes, please remove them. If no, I'd prefer them to be at least a 
> little bit analyzed.
>
> Jan
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
>   


-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/




More information about the sssd-devel mailing list