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

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

[DJ Delorie]

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!

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.  */
 
 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).  */
   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 */
+  /* This is the DLL function, like _nss_dns_gethostbyname_r().  */
+  void *fct_ptr; /* ptr-to-persistent */
 } known_function;
 
+/* Objects which may be unloaded at runtime and/or replaced.  */
 
 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 */
   /* 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.  */
   void *known;
   /* 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.  */
+  struct name_database_entry *next; /* ptr-to-unloadable */
+  /* List of services to be used.  */
+  service_user *service; /* ptr-to-unloadable */
+  /* Name of the database (`passwd', `hosts', etc).  */
   char name[0];
 } 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 */
   /* List of libraries with service implementation.  */
-  service_library *library;
+  service_library *library; /* ptr-to-persistent */
 } name_database;
 
 
+/* The rest of this header is not-data.  */
+
 #ifdef USE_NSCD
 /* Indices into DATABASES in nsswitch.c and __NSS_DATABASE_CUSTOM.  */
 enum

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

[Carlos O'Donell]

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
> 


-- 
Cheers,
Carlos.

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

[DJ Delorie]

"Carlos O'Donell" <codonell@redhat.com> writes:
>> +  /* 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)?

It's a link to, essentially, the next non-comment line in nsswitch.conf.
There are a lot of things labelled "next" in this header, keeping track
of next *what* is important.

We also don't have a lot of consistency and well-known terms for things
like "passwd or hosts" vs "dns or files" etc.

>> +/* The rest of this header is not-data.  */
>
> Remove this comment.

This is just a hint to me to stop reading the file ;-)

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

[Carlos O'Donell]

On 5/8/19 5:39 PM, DJ Delorie wrote:
> 
> "Carlos O'Donell" <codonell@redhat.com> writes:
>>> +  /* 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)?
> 
> It's a link to, essentially, the next non-comment line in nsswitch.conf.
> There are a lot of things labelled "next" in this header, keeping track
> of next *what* is important.

I agree. Please mention that in the comment.

> We also don't have a lot of consistency and well-known terms for things
> like "passwd or hosts" vs "dns or files" etc.
> 
>>> +/* The rest of this header is not-data.  */
>>
>> Remove this comment.
> 
> This is just a hint to me to stop reading the file ;-)

Ha! :-)

Please post a v2. I'd like to see this committed.

-- 
Cheers,
Carlos.

[V2 patch] Enhance comments in nsswitch.h

[DJ Delorie]

"Carlos O'Donell" <codonell@redhat.com> writes:
> Please post a v2. I'd like to see this committed.

V2 it is...

From 7696eca99b0f74b81dfb5af5848194db34ad1b4a Mon Sep 17 00:00:00 2001
From: DJ Delorie <dj@redhat.com>
Date: Tue, 14 May 2019 17:25:12 -0400
Subject: Document nsswitch.h more.

Add more comments to structures in nsswitch.conf to document their
current use, and potential reloadability.

diff --git a/ChangeLog b/ChangeLog
index cfdfcd57b7..0e3e2dd779 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2019-05-14  DJ Delorie  <dj@redhat.com>
+
+	* nss/nsswitch.h: Add more comments.
+
 2019-05-14  Florian Weimer  <fweimer@redhat.com>
 
 	Linux: Add the tgkill function.
diff --git a/nss/nsswitch.h b/nss/nsswitch.h
index 475e007e33..756db903b2 100644
--- a/nss/nsswitch.h
+++ b/nss/nsswitch.h
@@ -36,6 +36,19 @@ typedef enum
   NSS_ACTION_MERGE
 } lookup_actions;
 
+/* 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 (or at least need not) be unloaded:  */
 
 typedef struct service_library
 {
@@ -43,7 +56,9 @@ typedef struct service_library
   const char *name;
   /* Pointer to the loaded shared library.  */
   void *lib_handle;
-  /* And the link to the next entry.  */
+  /* 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).  */
   struct service_library *next;
 } service_library;
 
@@ -53,20 +68,28 @@ 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 */
+  /* This is the DSO function, like _nss_dns_gethostbyname_r().  */
+  void *fct_ptr; /* ptr-to-persistent */
 } known_function;
 
+/* The following structures generally hold data that may be unloaded
+   at runtime and/or replaced, such as if nsswitch.conf is
+   reloaded: */
 
 typedef struct service_user
 {
-  /* And the link to the next entry.  */
-  struct service_user *next;
+  /* 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 */
   /* 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
+     DSO, so at least one and at most a few mappings.  */
   void *known;
   /* Name of the service (`files', `dns', `nis', ...).  */
   char name[0];
@@ -78,11 +101,13 @@ 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.  */
+  /* The link to the next entry.  In a typical nsswitch.conf, this is
+     the passwd->group->hosts link, i.e. the next non-comment line in
+     the file.  */
+  struct name_database_entry *next; /* ptr-to-unloadable */
+  /* List of services to be used.  */
+  service_user *service; /* ptr-to-unloadable */
+  /* Name of the database (`passwd', `hosts', etc).  */
   char name[0];
 } name_database_entry;
 
@@ -90,9 +115,9 @@ 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 */
   /* List of libraries with service implementation.  */
-  service_library *library;
+  service_library *library; /* ptr-to-persistent */
 } name_database;
 
 

Re: [V2 patch] Enhance comments in nsswitch.h

[Florian Weimer]

* DJ Delorie:

> +   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.  */

What is a pointer to an unloadable object?  A DSO handle?  Or is it a
heap pointer that is never freed?

Is ptr-to-persistent to a static object?

Thanks,
Florian

Re: [V2 patch] Enhance comments in nsswitch.h

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
>> +   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.  */
>
> What is a pointer to an unloadable object?  A DSO handle?  Or is it a
> heap pointer that is never freed?

The intention is, if/when we allow for nsswitch.conf to be reloaded, we
need to understand which internal data *is* reloaded, and which *isn't*.

So ptr-to-unloadable means those malloc'd objects that can be free'd
when we reload nsswitch.conf.

> Is ptr-to-persistent to a static object?

Just to something that doesn't change once created.  The problem is, if
we load a DSO and some thread is using it via nss, and we reload
nsswitch.conf, we can't unload the DSO, because it might be in use.  So
ptr-to-persistent means it points to something that, once created, won't
go away.  Since the DSO doesn't change, any data mapping names to DSO
functions also won't need to change, etc.

Re: [V2 patch] Enhance comments in nsswitch.h

[Florian Weimer]

* DJ Delorie:

> Florian Weimer <fweimer@redhat.com> writes:
>>> +   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.  */
>>
>> What is a pointer to an unloadable object?  A DSO handle?  Or is it a
>> heap pointer that is never freed?
>
> The intention is, if/when we allow for nsswitch.conf to be reloaded, we
> need to understand which internal data *is* reloaded, and which *isn't*.
>
> So ptr-to-unloadable means those malloc'd objects that can be free'd
> when we reload nsswitch.conf.

But that doesn't seem true for e.g. service_user objects in the current
framework because threads traverse the list without locking.  We cannot
trivially add locking because we keep pointers service_user objects
across calls to NSS module functions, which can block for a long time.
Furthermore, nss_load_library writes a pointer the name from the
service_user object into the persistent service_library object, and
that's another obstacle to deallocation.

(service_user is even part of the ABI, but I'll try to come up with a
patch to change that.  unscd doesn't __nss_database_lookup which exposed
it.)

>> Is ptr-to-persistent to a static object?
>
> Just to something that doesn't change once created.  The problem is, if
> we load a DSO and some thread is using it via nss, and we reload
> nsswitch.conf, we can't unload the DSO, because it might be in use.

That's true for all data structures right now.  However, we shouldn't
dlclose any DSOs because they might not be prepared to be unloaded.

> So ptr-to-persistent means it points to something that, once created,
> won't go away.  Since the DSO doesn't change, any data mapping names
> to DSO functions also won't need to change, etc.

Okay.

I think the high-level question for me is whether this
can-be-deallocated property is a property of the struct member (where
you documented it in the patch), or a property of the type.  For
example, it seems to me that all name_database_entry objects can be
reloaded.  Maybe there is value in keeping this simple and not share
data too much?

Thanks,
Florian

Re: [V2 patch] Enhance comments in nsswitch.h

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> But that doesn't seem true for e.g. service_user objects in the current
> framework because threads traverse the list without locking.  We cannot

They key phrase here is "in the current framework".  Also in the current
framework, we don't reload nsswitch.conf.  We'd like to reload it.  That
means changing the code and dealing with these issues.  Analyzing the
objects that might need to be reloaded is merely the first step.

> trivially add locking because we keep pointers service_user objects
> across calls to NSS module functions, which can block for a long time.

Yup.  Perhaps we could temporarily have two configs running in parallel
until the old calls all finish.

> That's true for all data structures right now.  However, we shouldn't
> dlclose any DSOs because they might not be prepared to be unloaded.

Yeah, the idea was the DSO stuff would stick around, just the
nsswitch.conf rules would change.

> I think the high-level question for me is whether this
> can-be-deallocated property is a property of the struct member (where
> you documented it in the patch), or a property of the type.  For
> example, it seems to me that all name_database_entry objects can be
> reloaded.  Maybe there is value in keeping this simple and not share
> data too much?

Since I used "ptr-to-foo" my intent is to reflect if that pointer may
change across an nsswitch reload or not, which is the key information we
need to understand before changing code.  Indirectly, that implies which
types are reloadable, and the independent comments group those types
into persistent vs reloadable.

Part of the persistent-vs-reloadable stuff also comes from trying to
understand what information was actually stored in each type :-) For
example, we have a tree that maps API functions to DSO functions (the
"void *known;").  It's currently part of the nsswitch data, but the
mapping is DSO-specific.  We could move that map to the DSO data if we
accept that the map may contain more entries and thus increase
processing time but reduce memory cost.

Re: [V2 patch] Enhance comments in nsswitch.h

[Florian Weimer]

* DJ Delorie:

> Florian Weimer <fweimer@redhat.com> writes:
>> But that doesn't seem true for e.g. service_user objects in the current
>> framework because threads traverse the list without locking.  We cannot
>
> They key phrase here is "in the current framework".  Also in the current
> framework, we don't reload nsswitch.conf.  We'd like to reload it.  That
> means changing the code and dealing with these issues.  Analyzing the
> objects that might need to be reloaded is merely the first step.

I think we should not add comments which are misleading in the context
of the current implementation.

But documenting the current dependencies (e.g., which pointers to what),
and how things are accessed (with or without locks) still has value, of
course.

>> trivially add locking because we keep pointers service_user objects
>> across calls to NSS module functions, which can block for a long time.
>
> Yup.  Perhaps we could temporarily have two configs running in parallel
> until the old calls all finish.

It would also be possible to avoid deallocating the service_user objects
if we make sure that we re-use a previous list if it contains exactly
the same modules in the same configuration.  I think we only need to
avoid an unbounded leak if /etc/nsswitch.conf cycles between multiple
different configurations.  The number of different configuration a
process encounters during its life-time will be very small.  Even during
system installation, the number of different configurations will be
three or four at most.

Thanks,
Florian

Re: [V2 patch] Enhance comments in nsswitch.h

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> I think we should not add comments which are misleading in the context
> of the current implementation.

My initial plan wasn't to commit this at all, but to use it as a
discussion of how to refactor it all ;-)

Re: [V2 patch] Enhance comments in nsswitch.h

[Carlos O'Donell]

On 5/16/19 4:27 PM, DJ Delorie wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>> I think we should not add comments which are misleading in the context
>> of the current implementation.
> 
> My initial plan wasn't to commit this at all, but to use it as a
> discussion of how to refactor it all ;-)
> 

I think it can be both.

As a first pass you looked at which objects can be unloaded.

Florian and I discussed this further in private and the consensus
that emerged was that the following information could serve dual
purpose:

* Allocation ownership:
  - Who is allowed to allocate the object.
  - Who is allowed to deallocate the object.

* Usage pattern:
  - How does the ownership of the pointer get passed around
    the framework?

That is if instead of loading/unloading we focused on documenting
who owns the allocation and who can free it and the usage patterns,
that would actually highlight exactly where and which structures
can be unloaded. Since for example if you own the object and can
free it, then that's the only point where you could unload related
data.

-- 
Cheers,
Carlos.