[patch-ish,WIP] Enhance comments in nsswitch.h

In an attempt to understand how nsswitch.c operates, I've updated the
comments in nsswitch.h to act more as a guide-to-newbies.  As I'm also
working towards a reloadable nsswitch.conf, I've classified each
object as to whether it could be reloaded or not, and left in my
ptr-to-* comments despite them obviously being wrong-syntax.

I would greatly appreciate if anyone who understands nsswitch.conf
could review my comments and let me know if my understanding of the
code/objects is correct or not.

Thanks!

On 5/8/19 3:59 PM, DJ Delorie wrote:
> In an attempt to understand how nsswitch.c operates, I've updated the
> comments in nsswitch.h to act more as a guide-to-newbies.  As I'm also
> working towards a reloadable nsswitch.conf, I've classified each
> object as to whether it could be reloaded or not, and left in my
> ptr-to-* comments despite them obviously being wrong-syntax.

We are *creating* experts :-)

I worked with Stephen Gallagher on the MERGE functionality, and
have reviewed this code countless times. Even then I still feel
completely unqualified because it's not easy to review.

> I would greatly appreciate if anyone who understands nsswitch.conf
> could review my comments and let me know if my understanding of the
> code/objects is correct or not.
> 
> Thanks!

I think we should commit something like this when ready.
  
> diff --git a/nss/nsswitch.h b/nss/nsswitch.h
> index 475e007e33..4e0dad5b0c 100644
> --- a/nss/nsswitch.h
> +++ b/nss/nsswitch.h
> @@ -36,6 +36,7 @@ typedef enum
>     NSS_ACTION_MERGE
>   } lookup_actions;
>   

> +/* Objects which could be persistent once loaded.  */

What does this refer to? This should talk specifically about structures.

/* The NSS implementation uses a variety of data structures to
    implement the service plugin management.  Some of this data
    once loaded must be persistent.  Some of this data once loaded
    could subsequently be unloaded.  These comments are here to help
    guide a future implementation where we might unload this data.

    We define several types of pointers to try help identify the data:
    - ptr-to-malloc'd: Pointer to data which needs freeing.
    - ptr-to-persistent: Pointer to persistent object.
    - ptr-to-unloadable: Pointer to unloadlable object.  */

/* The following structures generally hold persistent data
    that cannot be unloaded:
    - ...  */

>   
>   typedef struct service_library
>   {
> @@ -43,7 +44,9 @@ typedef struct service_library
>     const char *name;
>     /* Pointer to the loaded shared library.  */
>     void *lib_handle;
> -  /* And the link to the next entry.  */
> +  /* And the link to the next entry, set by nss_new_service.  This
> +     link is used to free resources, and is not related to the order
> +     of handlers for each service (that is NEXT in service_user).  */

OK.

>     struct service_library *next;
>   } service_library;
>   
> @@ -53,20 +56,26 @@ typedef struct service_library
>      is the first member.  */
>   typedef struct
>   {
> -  const char *fct_name;
> -  void *fct_ptr;
> +  /* This is the API name, like `gethostbyname_r'.  */
> +  const char *fct_name; /* ptr-to-malloc'd */

OK.

> +  /* This is the DLL function, like _nss_dns_gethostbyname_r().  */

s/DLL/DSO/g.

OK.

> +  void *fct_ptr; /* ptr-to-persistent */
>   } known_function;
>   
> +/* Objects which may be unloaded at runtime and/or replaced.  */

This comment also should refer to the structures by name IMO.

Things get too easily lost during refactoring.

/* The following structures generally hold unloadable data
    that can be removed if we reload /etc/nsswitch.conf:
    - ...  */

>   
>   typedef struct service_user
>   {
> -  /* And the link to the next entry.  */
> -  struct service_user *next;
> +  /* And the link to the next entry.  In a line like `a : b c d' this
> +     is the b->c->d link.  */
> +  struct service_user *next; /* ptr-to-unloadable */

OK.

>     /* Action according to result.  */
>     lookup_actions actions[5];
>     /* Link to the underlying library object.  */
> -  service_library *library;
> -  /* Collection of known functions.  */
> +  service_library *library; /* ptr-to-persistent */
> +  /* Collection of known functions, mapping (for example) `foo' to
> +     _nss_dns_foo().  Only contains mappings for this service and this
> +     dll, so at least one and at most a few mappings.  */

s/dll/dso/g,

>     void *known;

Why don't we call it what it is? It's a binary search tree.

Since this is an internal header we can be as descriptive as we want.

>     /* Name of the service (`files', `dns', `nis', ...).  */
>     char name[0];
> @@ -78,11 +87,12 @@ typedef struct service_user
>   
>   typedef struct name_database_entry
>   {
> -  /* And the link to the next entry.  */
> -  struct name_database_entry *next;
> -  /* List of service to be used.  */
> -  service_user *service;
> -  /* Name of the database.  */
> +  /* And the link to the next entry.  In a typical nsswitch.conf, this
> +     is the passwd->group->hosts link.  */

What does this mean? That this is the next entry in the file after
translating it into a lit of entries (excluding comments)?

> +  struct name_database_entry *next; /* ptr-to-unloadable */
> +  /* List of services to be used.  */
> +  service_user *service; /* ptr-to-unloadable */

OK.

> +  /* Name of the database (`passwd', `hosts', etc).  */
>     char name[0];

OK. Ugh, vla.

>   } name_database_entry;
>   
> @@ -90,12 +100,14 @@ typedef struct name_database_entry
>   typedef struct name_database
>   {
>     /* List of all known databases.  */
> -  name_database_entry *entry;
> +  name_database_entry *entry; /* ptr-to-unloadable */

OK.

>     /* List of libraries with service implementation.  */
> -  service_library *library;
> +  service_library *library; /* ptr-to-persistent */

OK.

>   } name_database;
>   
>   
> +/* The rest of this header is not-data.  */

Remove this comment.

> +
>   #ifdef USE_NSCD
>   /* Indices into DATABASES in nsswitch.c and __NSS_DATABASE_CUSTOM.  */
>   enum
>