URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
pbrezina commented:
"""
Hi Nick,
thank you for the patches. The changes are not big, they are well structured
and commented. Thank you for that. You put a good work into it!
Here is the review. Please, do not be alarmed, it is only lots of quotation but
required changes are small and there isn't many of them.
* **CONFIG: Add session_recording section**
- options needs to go also into cfg_rules.ini for the validator
* **UTIL: Add session recording conf management module**
- `session_recording_conf_load()` -- please, check that the conf pointet is
not `NULL`
- I would welcome a little bit shorter `enum` values for scope, e.g.
`SESSION_RECORDING_SCOPE_NONE`. Or do you think that the `CONF` word is
important?
* **DP: Overlay sessionRecording attribute on initgr**
- Few minor styling issues:
```c
ctx->gids[ctx->gnum] = ldb_msg_find_attr_as_uint(res->msgs[i],
SYSDB_GIDNUM, 0);
^ we wrap to
parenthesis, i.e.:
SYSDB_GIDNUM, 0);
```
```c
groupname = (ctx->gnum == 0)
? NULL
: sss_nss_get_name_from_msg(ctx->domain_info,
res->msgs[i]);
It is not shorter and does not improve readibility, please use if/else
```
- Logic issues
```c
/* FIXME Do we need to/can we retrieve the primary group name? */
```
- Primary group is specified with `gidNumber` and suplementary groups are
specified with `memberUid`/`memberOf`. Primary group may or may not be part of
suplementary groups so yes, you need to special case it here.
```c
static errno_t dp_initgroups(struct sbus_request *sbus_req,
struct dp_client *dp_cli,
const char *key,
uint32_t dp_flags,
struct dp_id_data *data)
{
[...]
ret = sysdb_initgroups(sbus_req, domain, data->filter_value, &res);
if (ret == ENOENT || (ret == EOK && res->count == 0)) {
/* There is no point in concacting NSS responder. Proceed as usual. */
return EAGAIN;
^^^ we shortcut here and run request without postprocess function
} else if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get initgroups [%d]: %s\n",
ret, sss_strerror(ret));
goto done;
}
ctx = create_initgr_ctx(sbus_req, data->domain, domain, res);
if (ctx == NULL) {
ret = ENOMEM;
goto done;
}
dp_req_with_reply_pp(dp_cli, data->domain, "Initgroups", key,
sbus_req, DPT_ID, DPM_ACCOUNT_HANDLER, dp_flags, data,
dp_req_initgr_pp, ctx, struct dp_initgr_ctx,
dp_req_reply_std, struct dp_reply_std);
ret = EOK;
done:
talloc_free(res);
return ret;
}
```
- If initgroups was not yet performed for this user, we do not run the
postprocess function a proceed without it. I believe you want to set the
`sessionRecording` attribute even in this case so the postprocess function must
always be executed.
* **CACHE_REQ: Pull sessionRecording attrs from initgr**
```c
static errno_t cache_req_sr_overlay(struct tevent_req *req)
{
[...]
/* If we have group names to match against */
if (rctx->sr_conf.groups != NULL && *rctx->sr_conf.groups != NULL) {
^^^ please, use rctx->sr_conf.groups[0] != NULL so the intention is clearer
}
static errno_t cache_req_sr_overlay_match_users(struct tevent_req *req)
{
[...]
/* Create per-message talloc context */
tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) {
CACHE_REQ_DEBUG(SSSDBG_CRIT_FAILURE, cr,
"Failed creating temporary talloc context\n");
ret = ENOMEM;
goto done;
}
You can create one `tmp_ctx` for the whole function and then free only its
children in each cycle with `talloc_free_children()`. It will save some
allocations.
}
```
- Also please take this `cache_req_sr_overlay()` and move it into seperate
`cache_req_session_recording.c` (or similar) module and convert it to an
independent tevent request. This way, you are calling `tevent_req_done` inside
`cache_req_sr_overlay_match_all_step_done()` which is not very clean. So the
code in `cache_req.c` will change to something like this:
```c
static errno_t
cache_req_search_domains(struct tevent_req *req,
struct cache_req_domain *cr_domain,
bool check_next,
bool bypass_cache,
bool bypass_dp)
{
[...]
subreq = cache_req_search_domains_send(state, state->ev, state->cr,
cr_domain, check_next,
bypass_cache, bypass_dp);
if (subreq == NULL) {
return ENOMEM;
}
----tevent_req_set_callback(subreq, cache_req_done, req);
++++tevent_req_set_callback(subreq, cache_req_search_domains_done, req);
return EAGAIN;
}
static void
cache_req_search_domains_done(...)
{
...
subreq = cache_req_session_recording_send(...);
if (subreq == NULL) {
return ENOMEM;
}
tevent_req_set_callback(subreq, cache_req_done, req);
}
static void
cache_req_search_done(...)
{
cache_req_session_recording_recv()
...
}
```
* **NSS: Substitute session recording shell**
- Changes here looks like they may go into separate function.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/136#issuecomment-292153700
_______________________________________________
sssd-devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]
