Re: [CFT] Some updates to libc/rpc (second try)

On Fri, Aug 31, 2012 at 08:12:09AM -0400, John Baldwin wrote:
> On Friday, August 31, 2012 7:06:53 am Andrey Simonenko wrote:
> > On Thu, Aug 30, 2012 at 02:37:17AM -0700, Garrett Cooper wrote:
> > > > Detailed description of mistakes in these files and correct 
> implementation:
> > > >
> > > > http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/165710
> > > 
> > > A developer at $work (Isilon) developed a slightly simpler patch than
> > > that based on our custom 7.x sources recently to deal with concurrency
> > > issues in netconfig. I'll talk with a couple people to see whether or
> > > not the solution can be contributed back [after some polishing --
> > > maybe -- and further testing].
> > 
> > Can you post changes and corrected bugs in getnetconfig.c and getnetpath.c
> > in your (your $work) implementation.
> 
> There is a thread on threads_at_ with patches to make the API thread-safe
> that I believe came from Isilon.  It was posted in the last week or so,
> so it should be easy to find in the archives.
> 

Thank you for this information.

I checked the logic of their changes.

Making all data per-thread is wrong from my point of view.

1. Several threads can call getnetconfig() using the same handler obtained
   by setnetconfig().  If each thread has own FILE pointer, then some
   getnetconfig() will crash a program since fgets() will be called for NULL
   FILE pointer.

2. One thread can get handler by setnetconfig() and pass this handler to
   another thread and getnetconfig() will crash a program.  This is the
   similar mistake, but getnetconfig() is not called in parallel.

3. Each thread has to open netconfig(5) database, parse its content every
   time for each getnetconfig() call since data is not cached.  This is slow.

There is one per-thread value, it is error code of the last failed function,
this value cannot be kept in handler, since nc_perror() and nc_sperror()
do not have handler argument.   Since printing error is expected in the
same thread when getnetconfig() failed (for example), I think this is Ok.
If error is print in another thread, then we simply will not see correct
error message, but program will not be crashed.

I just commented only per-thread idea of data in getnetconfig.c, I do not
compare my implementation, since their patch does not correct any other
mistake described and corrected in my PR.