[PATCH 2/4] <nss_action.h>: New abstraction for combining NSS modules and NSS actions
Carlos O'Donell
carlos@redhat.com
Wed Jul 1 19:08:49 GMT 2020
On 6/25/20 12:05 AM, DJ Delorie via Libc-alpha wrote:
Please post v2.
- See suggested comments.
- See minor code changes requested in nss/nss_action.h
> ---
> nss/Makefile | 3 +-
> nss/nss_action.c | 114 ++++++++++++++++++++++++
> nss/nss_action.h | 92 +++++++++++++++++++
> nss/nss_action_parse.c | 197 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 405 insertions(+), 1 deletion(-)
> create mode 100644 nss/nss_action.c
> create mode 100644 nss/nss_action.h
> create mode 100644 nss/nss_action_parse.c
>
> diff --git a/nss/Makefile b/nss/Makefile
> index 5d357eb51e..464655d045 100644
> --- a/nss/Makefile
> +++ b/nss/Makefile
> @@ -28,7 +28,8 @@ headers := nss.h
> routines = nsswitch getnssent getnssent_r digits_dots \
> valid_field valid_list_field rewrite_field \
> $(addsuffix -lookup,$(databases)) \
> - compat-lookup nss_hash nss_module
> + compat-lookup nss_hash nss_module nss_action \
> + nss_action_parse
OK. New nss_action and nss_action_parse.
>
> # These are the databases that go through nss dispatch.
> # Caution: if you add a database here, you must add its real name
> diff --git a/nss/nss_action.c b/nss/nss_action.c
> new file mode 100644
> index 0000000000..24f1c5e4f9
> --- /dev/null
> +++ b/nss/nss_action.c
> @@ -0,0 +1,114 @@
> +/* NSS actions, elements in a nsswitch.conf configuration line.
OK.
> + Copyright (c) 2020 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <nss_action.h>
> +
> +#include <string.h>
> +#include <libc-lock.h>
> +
> +/* Maintain a global list of NSS action lists. Since most databases
> + use the same list of actions, this list is usually short.
> + Deduplication in __nss_action_allocate ensures that the list does
> + not grow without bounds. */
OK. This deduplication is important and keeps us bounded. If a roaming
device or network configuration change forces us to change between 1
or 2 or even 3 different configurations those cached configurations will
be reused without taking more memory.
> +
> +struct nss_action_list_wrapper
OK. Wraps the action list.
> +{
> + /* The next element of the list.x */
s/.x/./g
> + struct nss_action_list_wrapper *next;
OK. Next element.
> +
> + /* Number of elements in the list (excluding the terminator). */
> + size_t count;
OK.
> +
> + /* NULL-terminated list of actions. */
> + struct nss_action actions[];
OK.
> +};
> +
> +/* Global list of allocated NSS action lists. */
> +struct nss_action_list_wrapper *nss_actions;
OK. Global list.
> +
> +/* Covers the list. */
Which list?
Suggest:
/* Lock covers the nss_actions list. */
> +__libc_lock_define (static, nss_actions_lock);
> +
> +/* Returns true if the actions are equal (same module, same actions
> + aray). */
s/aray/array/g
> +static bool
> +actions_equal (const struct nss_action *a, const struct nss_action *b)
> +{
> + return a->module == b->module && a->action_bits == b->action_bits;
OK.
> +}
> +
> +/* Returns true if the COUNT actions at A and B are equal (according
> + to actions_equal above). */
Suggest:
/* Returns true if COUNT actions at A and B are equal (according
to actions_equal above). Caller must ensure that A and B have
at least COUNT actions. */
> +static bool
> +action_lists_equal (const struct nss_action *a, const struct nss_action *b,
> + size_t count)
> +{
> + for (size_t i = 0; i < count; ++i)
> + if (!actions_equal (a + i, b + i))
> + return false;
> + return true;
> +}
> +
> +/* Returns a pre-allocated action list for COUNT actions at ACTIONS,
> + or NULL if no such list exists. */
> +static nss_action_list
> +find_allocated (struct nss_action *actions, size_t count)
> +{
> + for (struct nss_action_list_wrapper *p = nss_actions; p != NULL; p = p->next)
> + if (p->count == count && action_lists_equal (p->actions, actions, count))
> + return p->actions;
> + return NULL;
> +}
OK.
> +
> +nss_action_list
> +__nss_action_allocate (struct nss_action *actions, size_t count)
> +{
> + nss_action_list result = NULL;
> + __libc_lock_lock (nss_actions_lock);
> +
> + result = find_allocated (actions, count);
OK. Look for a list we can use.
> + if (result == NULL)
> + {
OK. Didn't find one.
> + struct nss_action_list_wrapper *wrapper
> + = malloc (sizeof (*wrapper) + sizeof (*actions) * count);
OK. Allocate one of the right size.
> + if (wrapper != NULL)
> + {
> + wrapper->next = nss_actions;
> + wrapper->count = count;
> + memcpy (wrapper->actions, actions, sizeof (*actions) * count);
> + nss_actions = wrapper;
> + result = wrapper->actions;
> + }
> + }
> +
> + __libc_lock_unlock (nss_actions_lock);
> + return result;
OK. Return the allocated list or NULL on temporary allocation failure.
> +}
> +
> +void __libc_freeres_fn_section
> +__nss_action_freeres (void)
> +{
OK. Shutting down. Don't take the lock and walk the global list freeing
everything.
> + struct nss_action_list_wrapper *current = nss_actions;
> + while (current != NULL)
> + {
> + struct nss_action_list_wrapper *next = current->next;
> + free (current);
> + current = next;
> + }
> + nss_actions = NULL;
> +}
> diff --git a/nss/nss_action.h b/nss/nss_action.h
> new file mode 100644
> index 0000000000..7c0d3ef505
> --- /dev/null
> +++ b/nss/nss_action.h
> @@ -0,0 +1,92 @@
> +/* NSS actions, elements in a nsswitch.conf configuration line.
OK.
> + Copyright (c) 2020 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#ifndef _NSS_ACTION_H
> +#define _NSS_ACTION_H
> +
> +#include <stddef.h>
> +
> +#include "nsswitch.h" /* For lookup_actions. */
> +
> +struct nss_module;
> +
> +/* A NSS action pairs a service module with the action for each result
> + state. */
> +struct nss_action
> +{
> + /* The service module that provides the functionality (potentially
> + not yet loaded). */
> + struct nss_module *module;
OK.
> +
> + /* Action according to result. Two bits for each lookup_actions
> + value (from nsswitch.h), indexed by enum nss_status (from nss.h). */
> + unsigned int action_bits;
OK.
> +};
> +
> +/* Index in actions. */
Suggest:
/* Value to add to first nss_status value to get zero. */
#define NSS_STATUS_BIAS 2
/* Number of bits per lookup action. */
#define NSS_BPL 2
#define NSS_BPL_MASK ((1 << NSS_BPL) - 1)
/* Index in actions of an NSS status.
Note that in nss/nss.h the status starts at -2, and
we shift that up to zero by adding 2. Thus for example
NSS_STATUS_TRYAGAIN, which is -2, would index into the
0th bit place as expected. */
> +static inline int
> +nss_actions_bits_index (enum nss_status status)
> +{
> + return 2 * (2 + status);
Suggest:
return NSS_BPL * (NSS_STATUS_BIAS + status);
> +}
> +
> +/* Returns the lookup_action value for STATUS in ACTION. */
> +static inline lookup_actions
> +nss_action_get (const struct nss_action *action, enum nss_status status)
> +{
> + return (action->action_bits >> nss_actions_bits_index (status)) & 3;
Suggest:
return (action->action_bits >> nss_actions_bits_index (status)) & NSS_BPL_MASK;
> +}
OK.
> +
> +/* Sets the lookup_action value for STATUS in ACTION. */
> +static inline void
> +nss_action_set (struct nss_action *action,
> + enum nss_status status, lookup_actions actions)
> +{
> + int offset = nss_actions_bits_index (status);
> + unsigned int mask = 3 << offset;
Use NSS_BPL_MASK and then "mask" makes sense.
> + action->action_bits = ((action->action_bits & ~mask)
> + | ((unsigned int) actions << offset));
> +}
> +
> +static inline void
> +nss_action_set_all (struct nss_action *action, lookup_actions actions)
> +{
> + unsigned int bits = actions;
> + /* Produces 0b00 .. 00 AA AA AA AA AA. */
> + action->action_bits = bits * 0x155;
This needs to be rewritten to be expressed as a function of all the
nss_status entries. That is to say you need to call nss_action_set
for all status and actions. I dislike the above magic. It should be
immediately readable and understandable that we set all the entries.
> +}
> +
> +/* A list of struct nss_action objects in array terminated by an
> + action with a NULL module. */
> +typedef struct nss_action *nss_action_list;
OK.
> +
> +/* Returns a pointer to an allocated NSS action list that has COUNT
> + actions that matches the array at ACTIONS. */
> +nss_action_list __nss_action_allocate (struct nss_action *actions,
> + size_t count) attribute_hidden;
OK.
> +
> +/* Returns a pointer to a list allocated by __nss_action_allocate, or
> + NULL on error. ENOMEM means a (temporary) memory allocation error,
> + EINVAL means that LINE is syntactically invalid. */
> +nss_action_list __nss_action_parse (const char *line);
OK.
> +
> +/* Called from __libc_freeres. */
> +void __nss_action_freeres (void) attribute_hidden;
OK.
> +
> +
> +#endif /* _NSS_ACTION_H */
> diff --git a/nss/nss_action_parse.c b/nss/nss_action_parse.c
> new file mode 100644
> index 0000000000..ada3acbb08
> --- /dev/null
> +++ b/nss/nss_action_parse.c
> @@ -0,0 +1,197 @@
> +/* Parse a service line from nsswitch.conf.
OK.
> + Copyright (c) 1996-2020 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include "nss_action.h"
> +#include "nss_module.h"
> +
> +#include <ctype.h>
> +#include <string.h>
> +#include <stdbool.h>
> +
> +/* Staging area during parsing. */
> +#define DYNARRAY_STRUCT action_list
> +#define DYNARRAY_ELEMENT struct nss_action
> +#define DYNARRAY_PREFIX action_list_
> +#include <malloc/dynarray-skeleton.c>
> +
> +
> +/* Read the source names:
> + `( <source> ( "[" "!"? (<status> "=" <action> )+ "]" )? )*'
OK.
> + */
> +static bool
> +nss_action_parse (const char *line, struct action_list *result)
> +{
> + while (1)
> + {
> + while (isspace (line[0]))
> + ++line;
> + if (line[0] == '\0')
> + /* No source specified. */
> + return result;
> +
> + /* Read <source> identifier. */
> + const char *name = line;
> + while (line[0] != '\0' && !isspace (line[0]) && line[0] != '[')
> + ++line;
> + if (name == line)
> + return true;
> +
> + struct nss_action new_service
> + = { .module = __nss_module_allocate (name, line - name), };
OK. Got a new module. Allocate space for it.
> + if (new_service.module == NULL)
> + {
> + /* Memory allocation error. */
> + action_list_mark_failed (result);
> + return false;
> + }
> + nss_action_set_all (&new_service, NSS_ACTION_CONTINUE);
> + nss_action_set (&new_service, NSS_STATUS_SUCCESS, NSS_ACTION_RETURN);
> + nss_action_set (&new_service, NSS_STATUS_RETURN, NSS_ACTION_RETURN);
OK. Set some actions by default.
> +
> + while (isspace (line[0]))
> + ++line;
> +
> + if (line[0] == '[')
> + {
> + /* Read criterions. */
> + do
> + ++line;
> + while (line[0] != '\0' && isspace (line[0]));
> +
> + do
> + {
> + int not;
> + enum nss_status status;
> + lookup_actions action;
> +
> + /* Grok ! before name to mean all statii but that one. */
> + not = line[0] == '!';
> + if (not)
> + ++line;
> +
> + /* Read status name. */
> + name = line;
> + while (line[0] != '\0' && !isspace (line[0]) && line[0] != '='
> + && line[0] != ']')
> + ++line;
> +
> + /* Compare with known statii. */
> + if (line - name == 7)
> + {
> + if (__strncasecmp (name, "SUCCESS", 7) == 0)
> + status = NSS_STATUS_SUCCESS;
> + else if (__strncasecmp (name, "UNAVAIL", 7) == 0)
> + status = NSS_STATUS_UNAVAIL;
> + else
> + return false;
> + }
> + else if (line - name == 8)
> + {
> + if (__strncasecmp (name, "NOTFOUND", 8) == 0)
> + status = NSS_STATUS_NOTFOUND;
> + else if (__strncasecmp (name, "TRYAGAIN", 8) == 0)
> + status = NSS_STATUS_TRYAGAIN;
> + else
> + return false;
> + }
> + else
> + return false;
> +
> + while (isspace (line[0]))
> + ++line;
> + if (line[0] != '=')
> + return false;
> + do
> + ++line;
> + while (isspace (line[0]));
> +
> + name = line;
> + while (line[0] != '\0' && !isspace (line[0]) && line[0] != '='
> + && line[0] != ']')
> + ++line;
> +
> + if (line - name == 6 && __strncasecmp (name, "RETURN", 6) == 0)
> + action = NSS_ACTION_RETURN;
> + else if (line - name == 8
> + && __strncasecmp (name, "CONTINUE", 8) == 0)
> + action = NSS_ACTION_CONTINUE;
> + else if (line - name == 5
> + && __strncasecmp (name, "MERGE", 5) == 0)
> + action = NSS_ACTION_MERGE;
OK.
> + else
> + return false;
> +
> + if (not)
> + {
> + /* Save the current action setting for this status,
> + set them all to the given action, and reset this one. */
> + const lookup_actions save
> + = nss_action_get (&new_service, status);
> + nss_action_set_all (&new_service, action);
> + nss_action_set (&new_service, status, save);
OK.
> + }
> + else
> + nss_action_set (&new_service, status, action);
> +
> + /* Skip white spaces. */
> + while (isspace (line[0]))
> + ++line;
> + }
> + while (line[0] != ']');
> +
> + /* Skip the ']'. */
> + ++line;
> + }
> +
> + action_list_add (result, new_service);
OK.
> + }
> +}
> +
> +nss_action_list
> + __nss_action_parse (const char *line)
OK. Interfact to the rest of NSS.
> +{
> + struct action_list list;
> + action_list_init (&list);
OK. Setup DYNARRAY.
> + if (nss_action_parse (line, &list))
> + {
> + size_t size = action_list_size (&list);
> + nss_action_list result
> + = malloc (sizeof (*result) * (size + 1));
> + if (result == NULL)
> + {
> + action_list_free (&list);
> + return NULL;
> + }
> + memcpy (result, action_list_begin (&list), sizeof (*result) * size);
> + /* Sentinel. */
> + result[size].module = NULL;
> + return result;
> + }
> + else if (action_list_has_failed (&list))
> + {
> + /* Memory allocation error. */
> + __set_errno (ENOMEM);
OK.
> + return NULL;
> + }
> + else
> + {
> + /* Parse error. */
> + __set_errno (EINVAL);
OK.
> + return NULL;
> + }
> +}
>
--
Cheers,
Carlos.
More information about the Libc-alpha
mailing list