[PATCH 1/4] nss: Introduce <nss_module.h>

[DJ Delorie]

V2 to follow once I work through all four patch updates.

"Carlos O'Donell" <carlos@redhat.com> writes:
>> +/* Suffix after .so of NSS service modules.  */
>> +static const char *const __nss_shlib_revision = LIBNSS_FILES_SO + 15;
>
> This is magic.
>
> Suggest:
>
> /* Start with a known built-in provided service e.g. LIBNSS_FILES_SO
>    and extract the suffix after the of the NSS service modules e.g.
>    "libnss_files.so[.2]" => ".2".
>    We have no API to extract this except through the auto-generated
>    lib-names.h and some static pointer manipulation e.g. "+ 15".  */

Went with:

/* Suffix after .so of NSS service modules.  This is a bit of magic,
   but we assume LIBNSS_FILES_SO looks like "libnss_files.so.2" and we
   want a pointer to the ".2" part.  We have no API to extract this
   except through the auto-generated lib-names.h and some static
   pointer manipulation.  The "-1" accounts for the trailing NUL
   included in the sizeof.  */
static const char *const __nss_shlib_revision
	= LIBNSS_FILES_SO + sizeof("libnss_files.so") - 1;

>> +
>> +/* A single-linked list used to implement a mapping from names to NSS
>
> s/names/service names/g

Done.

>> +   modules.  (Most systems only use five or so service modules, so a
>
> s/service modules/modules/g

Done.

> Suggest:
>
> /* Allocate the service NAME with length NAME_LENGTH.  If the service
>    is already allocated in the nss_module_list cache then we return
>    a pointer to the struct nss_module, otherwise we try to allocate
>    a new struct nss_module entry and add it to the global nss_modules_list
>    cache.  If we fail to allocate the entry we return NULL.  Failure to
>    allocate the entry is always transient.  */

Changed.

>> +
>> +/* Long enough to store the name of any function.  */
>
> Suggest:
>
> /* Long enough to store the name of any function. Currently the
>    longest function is "getprotobynumber_r" which is 19 including
>    the NULL termiantor.  */
>
>> +typedef char function_name[19];
>
> Suggest:
>
> Should we change 19 to `sizeof("getprotobynumber_r")` to be statically
> determined by the compiler? This way it's clear what we're trying to do.
> It also avoids needing to count characters, you just updated by copy and
> pasting the new longest word.

Went with:

/* Long enough to store the name of any function in the
   nss_function_name_array list below, as getprotobynumber_r is the
   longest entry in that list.  */
typedef char function_name[sizeof("getprotobynumber_r")];


> Suggest:
>
> /* Internal implementation of __nss_module_load.  */

Changed.

> Suggest:
>
> /* Failing to load the module can be caused by several different scenarios.
>    One such scenario is that the module has been removed from the disk.
>    In which case the in-memory version is all that we have, and if the
>    module->state indidates it is loaded then we can use it.  */

Changed.

> Suggest:
> /* Copy the function pointers locally.  */

Went with:

  /* Look up and store locally all the function pointers we may need
     later.  Doing this now means the data will not change in the
     future.  */

>> +  /* Intall the function pointers, following the double-checked
>
> s/Intall/Install/g

Changed.

>
> Suggest:
>
> /* Synchronizes with unlocked __nss_module_load atomic_load_acquire.  */

Changed.


> Suggest:
>
> /* Turns out the module was already loaded, so close our own handle.
>    Closing our own handle does not actually unload the modules, only
>    the reference counter is decremented for the loaded module.  */

Went with:

      /* If the module was already loaded, close our own handle.  This
         does not actually unload the modules, only the reference
         counter is decremented for the loaded module.  */

>> +/* Ensures that MODULE is in a loaded or failed state.  */
>
> Suggest:
>
> /* Force the module identified by MODULE to be loaded.  We return false
>    if the module could not be loaded, true otherwise.  Loading the module
>    requires looking up all the possible interface APIs and caching the
>    results.  */

Changed.

> Suggest:
>
> /* Load module MODULE and return a pointer to the module's implementation
>    of NAME, otherwise return NULL on failure or error.  */

Went with:

/* Load module MODULE (if it isn't already) and return a pointer to
   the module's implementation of NAME, otherwise return NULL on
   failure or error.  */