Re: [PATCH] nsswitch: Add group merging support

[Stephen Gallagher <sgallagh at redhat dot com>]

On 01/12/2016 08:31 PM, Carlos O'Donell wrote:
> 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.
> 


Ping

I'd like to get this reviewed and merged in for 2.24 early on. Could someone
take another look?

Attachment: signature.asc
Description: OpenPGP digital signature