[SSSD] [PATCHES] Sysdb interface for netgroups

Stephen Gallagher sgallagh at redhat.com
Sun Sep 19 11:27:25 UTC 2010


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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

> Functions sysdb_getnetgr and sysdb_netgr_split_triple in src/db/sysdb_search.c 
> also allocate tmp context outside the hierarchy, I hope I didn't forget any 
> other occurence.
> I'm aware that these aren't causing any error right now, but for the future it 
> is safe to allocate the memory strictly in the correct parent context.
> 

See above.

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

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

I will fix that in the next version of the patch.

Thank you for the review, and I hope I have explained the memory
hierarchy sufficiently.

> I hope I have all that line numbers correct.
> 
> --
> Jan
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel


- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkyV850ACgkQeiVVYja6o6OvcACfQPIz672vnThUUnH0T2Kxxn4w
f7QAn0fLy+9M7ceH1JIlu5y4zDD7KqTJ
=wJZ6
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list