This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] nsswitch: Add group merging support


On 01/07/2016 07:46 PM, Stephen Gallagher wrote:
> 2015-12-16  Stephen Gallagher  <sgallagh@redhat.com>
> 
> 	[BZ #19072]
> 	* grp/Makefile (headers): Add grp-merge.h
> 	(routines): Add grp-merge.
> 	* grp/getgrgid_r.c: Include grp-merge.h.
> 	(DEEPCOPY_FN): Define.
> 	(MERGE_FN): Define.
> 	* grp/getgrname_r.c: Include grp-merge.h.
> 	(DEEPCOPY_FN): Define.
> 	(MERGE_FN): Define.
> 	* grp/grp-merge.c: New file.
> 	* grp/grp-merge.h: New file.
> 	* manual/nss.texi (Actions in the NSS configuration): Describe
> 	return, continue, and merge.
> 	* nscd/Makefile: Add vpath to find grp-merge.c
> 	(nscd-modules): Add grp-merge.
> 	* nscd/getgrgid_r.c: Include grp/grp-merge.h.
> 	(DEEPCOPY_FN): Define.
> 	(MERGE_FN): Define.
> 	* nscd/getgrnam_r.c: Include grp/grp-merge.h.
> 	(DEEPCOPY_FN): Define.
> 	(MERGE_FN): Define.
> 	* nss/getXXbyYY_r.c [!DEEPCOPY_FN]: Define __copy_einval.
> 	[!MERGE_FN]: Define __merge_einval.
> 	(CHECK_MERGE): Define.
> 	(REENTRANT_NAME): Process merge if do_merge is true.
> 	* nss/getnssent_r.c (__nss_setent): Process NSS_ACTION_MERGE.
> 	(__nss_getent_r): Likewise.
> 	* nss/nsswitch.c (nss_parse_service_list): Likewise.
> 	* nss/nsswitch.h (lookup_actions): Define NSS_ACTION_MERGE.

This patch looks good to me.

I reviewed this several times internally at Red Hat, and I think you
must have sent out the wrong version in the initial post because I'm
pretty sure I hammered out all the trivial code formatting issues
including the comment formatting :-)

Architecturally the patch is a good solution, and the merge buffers
are done in such a way that the new feature is clean and easy to
implement.

>From an implementation perspective I think the documentation of the
feature and the implementation are done and well tested by you in your
environment, and we're working on a broader set of tests which will
provide regression testing for this feature. It's not yet done, but
it uses completely uncontroversial features like __nss_configure_lookup
and two test NSS modules.

I think this should do into 2.23 as a new feature before the freeze.

It has little to no risk because it is not a public ABI/API issue.

The scenario for systems with [SUCCESS=merge] without this support
is fail-safe, and it limits the lookup scope.

I'll propose to Adhemerval that we merge this for 2.23.

Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]