On Tue, 2012-01-31 at 22:04 -0500, Stephen Gallagher wrote:
On Tue, 2012-01-31 at 22:15 +0100, Jan Zeleny wrote:
> Jan Zelený <jzeleny(a)redhat.com> wrote:
> > #0008:
> > SELinux user maps support in IPA provider. Also generig data provider
> > related code is here. I'm considering splitting this patch in two or
> > three. Let me know your opinion.
> >
Too much code to look over at 10pm. Will get to it tomorrow.
First point: yes, this needs to be at least two patches. The first
should create the common session creation functionality. The second
should implement it in the IPA provider.
Please reorder ipa_session.c so that the _recv() functions follow the
_send/<steps>/_done functions for a tevent request.
ipa_get_selinux_recv() does not belong between ipa_session_handler() and
ipa_get_selinux_done().
Also, your naming of ipa_get_selinux_done() is wrong. Call it something
like ipa_session_handler_done(). It's not part of the
ipa_get_selinux_*() tevent_req, so don't name it like it is.
In ipa_get_selinux_done() you have FIXME to add an outer transaction.
Please do this. Both sysdb_store_selinux_config() and
ipa_save_user_maps() are synchronous actions, so it's perfectly safe.
Please don't use numeric DEBUG levels any more. Use the appropriate
macro level. For example, in ipa_get_selinux_send(), the connection
status should probably be SSSDBG_TRACE_INTERNAL, not level 9 (AKA
SSSDBG_TRACE_ALL). SSSDBG_TRACE_ALL should be reserved for REALLY
esoteric data (like pointer addresses in the sbus connection code).
In ipa_get_selinux_send(), I'd rather you handled the label like this:
...
return req;
immediate:
if (ret == EOK) {
tevent_req_done(req);
} else {
tevent_req_error(req, ret);
}
tevent_req_post(req, ev);
return req;
}
Furthermore, I don't think we want to return success for offline. You
should add support for dp_err, errnop and err_msg to the
ipa_get_selinux_state and pass those back in ipa_get_selinux_recv().
Then extend ipa_session_reply() to return these values. This way you
will properly return DP_ERR_OFFLINE and how to handle that will belong
to the responder. You should handle this appropriately in each of the
step functions here as well.
I just made this same comment on a private review of the SSH key patches
in progress:
"Please do not reimplement ipa_get_hosts_send() completely. Please
modify ipa_hbac_host_info_send() so that you can maintain a common
routine. (You should be able to just add a boolean to skip the hostgroup
lookup and whatever new attributes you need)"
Please work with Honza (jcholast(a)redhat.com) to come up with a common
routine that all three features (SELinux, SSH and HBAC) can use.
ipa_selinux_get_maps_state should not be public. Proper encapsulation
means that it should only ever be visible to the
ipa_selinux_get_maps_send() tevent_req. If any external application
needs data from it, they should be getting it from
ipa_selinux_get_maps_recv();
If you get to the done: label in ipa_get_selinux_maps_done() via
receiving no selinux maps, you're not calling tevent_req_done(). Thus
the request will never complete.
I have the same concern here about the greedy array allocation that I
had in the sysdb code. There could be hundreds (or thousands) of
possible user maps to filter through. I don't want to waste that much
memory on the confirmed and possible matches.
Wrap the call to ipa_get_config_send() in a function to be used in both
ipa_get_selinux_maps_done() and ipa_get_selinux_hbac_done().