Re: [CFT] Some updates to libc/rpc (second try)
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. This is the list of changes and corrected bugs in my implementation: 1. __nc_error() does not check return value from malloc() and can pass NULL pointer to thr_setspecific(). 2. setnetconfig() has a race condition with reference counter when several threads call this function and only one thread successfully opened database file, while other threads failed in this function. If that one thread called endnetconfig(), then it can keep cached data in memory, but all threads will not have opened handles. 3. getnetconfig() should return entries from /etc/netconfig file in the same order as they are specified according to netconfig(5). If several threads call getnetconfig() using different handlers, then entries for each handler will be returned in random order. 4. getnetconfig() has a race condition that can cause NULL pointer dereference when several threads call this function using one handler. 5. getnetconfig() allows to continue to get entries if database file has invalid format, because it does not remember previous state. 6. endnetconfig() has a race condition with reference count and can keep cached data, while all handlers are closed. 7. getnetconfigent() uses getnetconfig() and has the same mistakes, also this function duplicates code from getnetconfig(). 8. getnetconfig() and getnetconfigent() use too much memory for entry data, each entry require ~1 kbytes of memory, while usually only 50 bytes is needed. 9. parse_ncp() incorrectly parses flags in netconfig entry and allows wrong combinations of flags, it does not allow spaces before entry, does not check number of elements in each netconfig entry, does not allow empty lines. 10. nc_sperror() is not optimal. 11. dup_ncp() is not optimal, allocates more memory than was used in the original structure, call strcpy() several times instead of calling memcpy() one time. 12. setnetpath() is not optimal, e.g. it calls setnetconfig() and then calls endnetconfig(). 13. getnetpath() uses getnetconfig() and getnetconfigent() and has the same mistakes. 14. getnetpath() has race conditions when several threads call this function using one handler, as a result there are memory leaks and not synchronized access with modifications to data if the NETPATH environment variable is set. 15. _get_next_token() is too complex, incorrectly understand \-sequences. 16. All functions do not specify error code in all possible cases, so nc_sperror() and nc_perror() functions are useless. Difference between netconfig.c vs getnetconfig.c and getnetpath.c: 1. __nc_error() was corrected, but its implementation is the same, this is a standard implementation for thread-specific data handling. nc_perror() was taken from getnetconfig.c, it cannot be written in other way. 2. Some errors messages were taken from getnetconfig.c. 3. New nc_parse() (old parse_ncp()) was corrected and optimized a bit, it just parses white space separated fields in a string. 4. Some variables and macro variables names were taken from getnetconfig.c. 5. All other functions and data structures were rewritten. Additionally I corrected libc/include/reentrant.h, getnetconfig.3, and getnetpath.3. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [CFT] Some updates to libc/rpc (second try)
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@ 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. -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
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@ 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. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [CFT] Some updates to libc/rpc (second try)
On Wed, Aug 29, 2012 at 09:57:37PM -0500, Pedro Giffuni wrote: This is rather critical stuff (libc) so I have no hurry and would like extensive testing before considering it for head. Please give it a try and report any issue. Looks like that their getnetconfig.c and getnetpath.c have similar mistakes as these files have in FreeBSD. Detailed description of mistakes in these files and correct implementation: http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/165710 Related PR: http://www.freebsd.org/cgi/query-pr.cgi?pr=79683 ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [CFT] Some updates to libc/rpc (second try)
On Thu, Aug 30, 2012 at 2:21 AM, Andrey Simonenko si...@comsys.ntu-kpi.kiev.ua wrote: On Wed, Aug 29, 2012 at 09:57:37PM -0500, Pedro Giffuni wrote: This is rather critical stuff (libc) so I have no hurry and would like extensive testing before considering it for head. Please give it a try and report any issue. Looks like that their getnetconfig.c and getnetpath.c have similar mistakes as these files have in FreeBSD. 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]. Related PR: http://www.freebsd.org/cgi/query-pr.cgi?pr=79683 Thanks! -Garrett ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
[CFT] Some updates to libc/rpc (second try)
(Second try with a more benign mailer) Hello; A while back the Bull NFS4 for linux project took our RPC support from libcand did some enhancements on it. The libraries have been diverging extensively and many of the changes are linux specific. The complete log of their changes is here: http://git.infradead.org/users/steved/libtirpc.git/log I don't think it would be a good idea to consider them upstream vendors, but I thought it would be good to attempt to keep partially in sync. I only took a rather small subset of their changes and got to this patch: http://people.freebsd.org/~pfg/patches/patch-rpc http://people.freebsd.org/%7Epfg/patches/patch-rpc For those interested in the changelog, it's here: http://people.freebsd.org/~pfg/patches/libtirpc-log http://people.freebsd.org/%7Epfg/patches/libtirpc-log This is rather critical stuff (libc) so I have no hurry and would like extensive testing before considering it for head. Please give it a try and report any issue. Pedro. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
