This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] nsswitch: Add group merging support
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Stephen Gallagher <sgallagh at redhat dot com>, libc-alpha at sourceware dot org
- Date: Tue, 12 Jan 2016 20:31:08 -0500
- Subject: Re: [PATCH] nsswitch: Add group merging support
- Authentication-results: sourceware.org; auth=none
- References: <871t9ul4az dot fsf at igel dot home> <1452213991-6499-1-git-send-email-sgallagh at redhat dot com>
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.