Stephen Gallagher <sgallagh(a)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