On 10/15/2014 01:08 PM, Sumit Bose wrote:
On Tue, Oct 14, 2014 at 05:57:36PM +0200, Sumit Bose wrote:
> Hi,
>
> this patches make the nss responder view aware. A number of new sysdb
> calls is added as drop in replacements from existing ones. I preferred
> this instead of modifying the existing ones to not break their usage in
> other places. But I think they are a good targets, together with the
> other user and group related sysdb calls, for a refactoring in an
> upcoming version.
>
> bye,
> Sumit
Please find attached a new version with some minor fixes in fill_pwent()
and fill_grent() with respect to fully qualified names and an additional
patch with the "changes" for the pam responder.
bye,
Sumit
Patch #4 sysdb: add overide lookup calls won't apply to current master.
But the rebase was trivial so it didn't stop me from reviewing.
Patch #1: confdb: add has_views and view_name to sss_domain_info
ACK
Patch #2: new_subdomain: copy view data from parent
ACK
Patch #3: sysdb: add view data to domains
+ if (ret == ENOENT || view_name == NULL
+ || (view_name != NULL
+ && strcmp(view_name, SYSDB_DEFAULT_VIEW_NAME) == 0)) {
view_name != NULL condition can be skipped
And the whole branching in sysdb_master_domain_update() is quite complex
and difficult to understand. Can you add a comment to the top of each
branch that says when the branch will be executed?
Patch #4: sysdb: add overide lookup calls
ACK
Patch #5: sysdb: add sysdb_getpwnam/uid_with_views()
+ /* TODO: check if EOK is returned even if orig_obj is empty, i.e
nothgin
+ * found. */
This is strange todo to leave in the code. Can you do it? :-)
Patch #6: sysdb: add sss_view_ldb_msg_find_element/attr_as_string/uint64
ACK
Patch #7: nss: add view support for getpwnam/getpwuid requests
I think you can use sss_view_ldb* functions from the previous patch,
can't you?
Patch #8: sysdb: add sysdb_initgroups_with_views()
ACK
Patch #9: nss: add view support to initgroups request
ACK
Patch #10: sysdb: add sysdb_getgrnam_with_views and
sysdb_getgrgid_with_views
+ /* TODO: check if EOK is returned even if orig_obj is empty, i.e
nothing
+ * found. */
Again.
Patch #11: nss: add view support for getgr* requests
Coverity warning:
nsssrv_cmd.c:2856: value_overwrite: Value "22" is overwritten with value
from "sss_packet_set_size(packet, 0UL)".
nsssrv_cmd.c:2810: assigned_value: Value "22" is assigned to "ret"
here,
but that stored value is not used before it is overwritten.
Patch #12: sid2name: return name without views applied
+ if (apply_no_view) {
+ orig_name = ldb_msg_find_attr_as_string(msg,
+ ORIGINALAD_PREFIX SYSDB_NAME,
+ NULL);
+ }
+
+ if (orig_name == NULL) {
+ orig_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
+ }
The name of the variable 'apply_no_view' suggests that if true, no
override should be applied. Thus if false, shouldn't we use
sss_view_ldb* to get overriden value if possible?
Patch #13: pam: make pam responder aware if views
ACK
I was using these patches for quite some time during my work on IFP and
sudo responder part of the views. All known issues seems to be resolved.
Functional ACK.