NSS: Use netgr as memory context in set_netgr_lifetime() - Patch for SSSD-1.14 #444
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
Ack. Just the commit message still talkes about |
Contributor
Author
|
@pbrezina, thanks for the review. I've changed the commit message. |
Contributor
Author
|
Adding "Accepted" label per @pbrezina's review. |
lslebodn
reviewed
Nov 13, 2017
src/responder/nss/nsssrv_netgroup.c
Outdated
| tv = tevent_timeval_current_ofs(lifetime, 0); | ||
| te = tevent_add_timer(step_ctx->nctx->rctx->ev, | ||
| step_ctx->nctx->gctx, tv, | ||
| netgr, tv, |
Contributor
There was a problem hiding this comment.
Why did you decide to use netgr as a context?
IMHO it is cleaner to use struct nss_ctx instead of struct getent_ctx
fidencio
force-pushed
the
wip/nss_setnetgrent_timeout_1_14_patch
branch
from
67f739d to
9070d59
Compare
Contributor
Author
|
Changes done, thanks for the review. |
We've noticed some crashes that happened because gctx is already freed, but the timeout handler is still called. In order to avoid that, let's remove the timeout handler when nss_ctx is freed at other places. Resolves: https://pagure.io/SSSD/sssd/issue/3523 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
fidencio
force-pushed
the
wip/nss_setnetgrent_timeout_1_14_patch
branch
from
9070d59 to
f38e83a
Compare
Member
|
On 11/13/2017 06:11 PM, lslebodn wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/responder/nss/nsssrv_netgroup.c
<#444 (comment)>:
> @@ -428,7 +428,7 @@ static void set_netgr_lifetime(uint32_t lifetime,
tv = tevent_timeval_current_ofs(lifetime, 0);
te = tevent_add_timer(step_ctx->nctx->rctx->ev,
- step_ctx->nctx->gctx, tv,
+ netgr, tv,
Why did you decide to use |netgr| as a context?
IMHO it is cleaner to use |struct nss_ctx| instead of |struct getent_ctx|
The timer will free `netgr`. IMHO it is correct to cancel the timer when
`netgr` is no longer available therefore using it as memory context is
correct.
|
Contributor
|
On (14/11/17 10:20), Pavel Březina wrote:
On 11/13/2017 06:11 PM, lslebodn wrote:
> ***@***.**** commented on this pull request.
>
> ------------------------------------------------------------------------
>
> In src/responder/nss/nsssrv_netgroup.c
> <#444 (comment)>:
>
>> @@ -428,7 +428,7 @@ static void set_netgr_lifetime(uint32_t lifetime,
>
> tv = tevent_timeval_current_ofs(lifetime, 0);
> te = tevent_add_timer(step_ctx->nctx->rctx->ev,
> - step_ctx->nctx->gctx, tv,
> + netgr, tv,
>
> Why did you decide to use |netgr| as a context?
>
> IMHO it is cleaner to use |struct nss_ctx| instead of |struct getent_ctx|
The timer will free `netgr`. IMHO it is correct to cancel the timer when
`netgr` is no longer available therefore using it as memory context is
correct.
All structures `getent_ctx` are allocated on `nctx`. So it is very minimal
change and it is closes to change in master
```
sh$ git grep "struct getent_ctx" | grep talloc
src/responder/nss/nsssrv_cmd.c: state->nctx->pctx = talloc_zero(nctx, struct getent_ctx);
src/responder/nss/nsssrv_cmd.c: state->nctx->gctx = talloc_zero(nctx, struct getent_ctx);
src/responder/nss/nsssrv_netgroup.c: *netgr = talloc_get_type(value.ptr, struct getent_ctx);
src/responder/nss/nsssrv_netgroup.c: talloc_get_type(ctx, struct getent_ctx);
src/responder/nss/nsssrv_netgroup.c: state->netgr = talloc_zero(nctx, struct getent_ctx);
src/responder/nss/nsssrv_netgroup.c: netgr = talloc_zero(step_ctx->nctx, struct getent_ctx);
src/responder/nss/nsssrv_netgroup.c: talloc_get_type(pvt, struct getent_ctx);
src/responder/nss/nsssrv_netgroup.c: netgr = talloc_get_type(item->value.ptr, struct getent_ctx);
src/responder/nss/nsssrv_services.c: state->nctx->svcctx = talloc_zero(nctx, struct getent_ctx);
```
LS
|
Contributor
|
@pbrezina can it be pushed? |
Member
|
You have already pushed version with |
Contributor
|
On (15/11/17 11:54), Pavel Březina wrote:
You have already pushed version with `netgr` into 1.13 (#445). I don't think we need to have three different patches in three different sssd versions.
May I know where it was pushed.
I cannot see it in upstream
https://pagure.io/SSSD/sssd/commits/sssd-1-13
ATM there is only patch in master f6a1cef
https://pagure.io/SSSD/sssd/commits/master
LS
|
Member
|
Ah, sorry, you haven't pushed it, just closed it. My bad. Having it on |
Contributor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We've noticed some crashes that happened because netgr is already freed,
but the timeout handler is still called. In order to avoid that, let's
remove the timeout handler when enum_ctx is freed at other places.
Resolves: https://pagure.io/SSSD/sssd/issue/3523
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com