[SSSD] [PATCHES] Sysdb interface for netgroups

Jan Zelený jzeleny at redhat.com
Mon Sep 20 10:25:30 UTC 2010


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.

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



More information about the sssd-devel mailing list