On Fri, Oct 17, 2014 at 02:05:44PM +0200, Pavel Březina wrote:
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.
Thank you very much for the review.
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
dropped
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?
done
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? :-)
removed
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?
yes, except for name and gid where some special handling is needed. For
the primary gid I assume that a server-side override overrides a
client-side value given by the override_gid option.
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.
removed
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.
fixed
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?
yes, although I used ldb_msg_find_attr_as_string and look for the
override value directly to make to more clear what is happening and
since override names are un-qualified they need some special
handling.
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.
New version attached.
bye,
Sumit
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel