From owner-freebsd-threads@FreeBSD.ORG Mon Aug 20 11:08:06 2012 Return-Path: Delivered-To: freebsd-threads@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 4579F1065675 for ; Mon, 20 Aug 2012 11:08:06 +0000 (UTC) (envelope-from owner-bugmaster@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 1582F8FC0C for ; Mon, 20 Aug 2012 11:08:06 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.5/8.14.5) with ESMTP id q7KB85Y9048195 for ; Mon, 20 Aug 2012 11:08:05 GMT (envelope-from owner-bugmaster@FreeBSD.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.5/8.14.5/Submit) id q7KB85HJ048176 for freebsd-threads@FreeBSD.org; Mon, 20 Aug 2012 11:08:05 GMT (envelope-from owner-bugmaster@FreeBSD.org) Date: Mon, 20 Aug 2012 11:08:05 GMT Message-Id: <201208201108.q7KB85HJ048176@freefall.freebsd.org> X-Authentication-Warning: freefall.freebsd.org: gnats set sender to owner-bugmaster@FreeBSD.org using -f From: FreeBSD bugmaster To: freebsd-threads@FreeBSD.org Cc: Subject: Current problem reports assigned to freebsd-threads@FreeBSD.org X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Aug 2012 11:08:06 -0000 Note: to view an individual PR, use: http://www.freebsd.org/cgi/query-pr.cgi?pr=(number). The following is a listing of current problems submitted by FreeBSD users. These represent problem reports covering all versions including experimental development code and obsolete releases. S Tracker Resp. Description -------------------------------------------------------------------------------- o threa/170073 threads [patch] stdatomic.h doesn't use clang builtins for C o threa/168417 threads pthread_getcpuclockid() does not work to specification o threa/165173 threads [build] clang buildworld breaks libthr o threa/163512 threads libc defaults to single threaded o threa/160708 threads possible security problem with RLIMIT_VMEM o threa/150959 threads [libc] Stub pthread_once in libc should call _libc_onc o threa/148515 threads Memory / syslog strangeness in FreeBSD 8.x ( possible o threa/141721 threads rtprio(1): (id|rt)prio priority resets when new thread o threa/135673 threads databases/mysql50-server - MySQL query lock-ups on 7.2 o threa/128922 threads threads hang with xorg running o threa/122923 threads 'nice' does not prevent background process from steali o threa/121336 threads lang/neko threading ok on UP, broken on SMP (FreeBSD 7 o threa/116668 threads can no longer use jdk15 with libthr on -stable SMP o threa/115211 threads pthread_atfork misbehaves in initial thread o threa/110636 threads [request] gdb(1): using gdb with multi thread applicat o threa/110306 threads apache 2.0 segmentation violation when calling gethost o threa/103975 threads Implicit loading/unloading of libpthread.so may crash o threa/101323 threads [patch] fork(2) in threaded programs broken. o threa/80992 threads abort() sometimes not caught by gdb depending on threa o threa/79683 threads svctcp_create() fails if multiple threads call at the s threa/76694 threads fork cause hang in dup()/close() function in child (-l s threa/30464 threads [patch] pthread mutex attributes -- pshared 22 problems total. From owner-freebsd-threads@FreeBSD.ORG Fri Aug 24 20:00:28 2012 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 99A5A1065670 for ; Fri, 24 Aug 2012 20:00:28 +0000 (UTC) (envelope-from steven.haber@isilon.com) Received: from mexforward.lss.emc.com (hop-nat-141.emc.com [168.159.213.141]) by mx1.freebsd.org (Postfix) with ESMTP id 2C1B28FC1E for ; Fri, 24 Aug 2012 20:00:27 +0000 (UTC) Received: from hop04-l1d11-si01.isus.emc.com (HOP04-L1D11-SI01.isus.emc.com [10.254.111.54]) by mexforward.lss.emc.com (Switch-3.4.3/Switch-3.4.3) with ESMTP id q7OK0KWK026223 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 24 Aug 2012 16:00:21 -0400 Received: from mailhub.lss.emc.com (mailhubhoprd03.lss.emc.com [10.254.221.145]) by hop04-l1d11-si01.isus.emc.com (RSA Interceptor); Fri, 24 Aug 2012 16:00:04 -0400 Received: from seacasht02.desktop.isilon.com (seacasht02.isilon.com [137.69.159.81]) by mailhub.lss.emc.com (Switch-3.4.3/Switch-3.4.3) with ESMTP id q7OK03t9032229; Fri, 24 Aug 2012 16:00:03 -0400 Received: from seaxch01.isilon.com (137.69.158.60) by SEACASHT02.desktop.isilon.com (137.69.159.81) with Microsoft SMTP Server id 14.2.298.4; Fri, 24 Aug 2012 13:00:02 -0700 X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-Class: urn:content-classes:message MIME-Version: 1.0 Date: Fri, 24 Aug 2012 12:58:45 -0700 Message-ID: <56CE86D6660FF84498426FA7A33170B40413DF8E@seaxch01.desktop.isilon.com> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: netconfig is not threadsafe Thread-Index: Ac2CMt5V549bHjBFQPKks/Vvv9t6ZA== From: Steven Haber To: , X-EMM-MHVC: 1 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.5 Cc: Subject: netconfig is not threadsafe X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Aug 2012 20:00:28 -0000 netconfig (libc routines for reading the netconfig file) is currently not threadsafe. It is a transactional API set, that is, you call setnetconfig() to open the file, read from the file with getnetconfig(), and close/cleanup with endnetconfig(). The problem is that the order that these routines are called in is important, but the state of the current transaction is stored in a per-process static, so only a single thread can do a transaction at one time. A patch from five years ago attempted to fix this: =20 http://lists.freebsd.org/mailman/confirm/freebsd-threads/292f06b38808587 41c9034fb4173a14740c63a9e =20 This patch does not solve the problem. It adds protection for the statics so that concurrent calls into set/get/endnetgrent() don't trash the statics. However, there is no use case where you would want two threads accessing any of these statics concurrently. To solve this problem, I have stored the FILE and netconfig_info statics on a per-thread basis using the pthreads key setup that was already in place for the error code. Now threads have their own set of stored state data, so no statics are necessary. I removed the original locking that had no effect. =20 My repro is to spawn 32 threads that all create an RPC client simultaneously using libc's clnt_create(), which calls into netconfig. After patching, I no longer see the issue. Here is the stack when a thread collision occurs (same as in the original bug from 2007): =20 #0 fclose (fp=3D0x0) at /root/Source/onefs/git/src/lib/libc/stdio/fclose.c:52 #1 0x0000000802c83a79 in endnetconfig (handlep=3D0x80b1030a0) at /root/Source/onefs/git/src/lib/libc/rpc/getnetconfig.c:433 #2 0x0000000802c811d0 in __rpc_endconf (vhandle=3D0x80b103090) at /root/Source/onefs/git/src/lib/libc/rpc/rpc_generic.c:445 #3 0x0000000802c7331a in clnt_create_timed (hostname=3D0x804d7cd9b "tgroup %s", prog=3D134420864, vers=3D1, netclass=3DVariable "netclass" = is not available. ) at /root/Source/onefs/git/src/lib/libc/rpc/clnt_generic.c:271 =20 This issue is present in FreeBSD head, though I'm reproducing on 7.1. =20 Does the patch make sense? Should I open a bug on this somehow? I'm relatively new to contributing to FreeBSD. =20 Thanks, Steven =20 =20 >From f3639afbe7993f3c699f23f4d77749ea01e8e8c8 Mon Sep 17 00:00:00 2001 From: Steven Haber Date: Thu, 23 Aug 2012 17:45:51 -0700 Subject: [PATCH] Fix for bug 95757. Make netconfig threadsafe so we can make lots of RPC clients concurrently. The original locking had no effect; there is no point in protecting the static members since you should never have two threads in the same get/set/endnetconfig. The point of these functions is to be called in sequence as a transaction. You can't have multiple threads calling them if the state is stored as a process global. This fix moves the static vars to thread- specific storage so that each thread can have its own transaction, and removes the exiting locking. --- src/lib/libc/rpc/getnetconfig.c | 74 +++++++++++---------------------------- 1 file changed, 21 insertions(+), 53 deletions(-) =20 diff --git a/src/lib/libc/rpc/getnetconfig.c b/src/lib/libc/rpc/getnetconfig.c index 0f7168f..2957d14 100644 --- a/src/lib/libc/rpc/getnetconfig.c +++ b/src/lib/libc/rpc/getnetconfig.c @@ -119,22 +119,21 @@ struct netconfig_vars { struct netconfig_list *nc_configs; /* pointer to the current netconfig entry */ }; +struct nc_thread_vars { + int nc_error; + FILE *nc_file; + struct netconfig_info nc_ni; +}; + #define NC_VALID 0xfeed #define NC_STORAGE 0xf00d #define NC_INVALID 0 =20 -static int *__nc_error(void); static int parse_ncp(char *, struct netconfig *); static struct netconfig *dup_ncp(struct netconfig *); =20 -static FILE *nc_file; /* for netconfig db */ -static mutex_t nc_file_lock =3D MUTEX_INITIALIZER; - -static struct netconfig_info ni =3D { 0, 0, NULL, NULL}; -static mutex_t ni_lock =3D MUTEX_INITIALIZER; - static thread_key_t nc_key; static once_t nc_once =3D ONCE_INITIALIZER; static int nc_key_error; @@ -148,34 +147,37 @@ nc_key_init(void) #define MAXNETCONFIGLINE 1000 -static int * -__nc_error() +static struct nc_thread_vars * +__nc_thread() { - static int nc_error =3D 0; - int *nc_addr; + static struct nc_thread_vars nc_vars =3D {0, NULL, {0, 0, NULL, NULL}}; + struct nc_thread_vars *nc_addr; /* - * Use the static `nc_error' if we are the main thread + * Use the static `nc_vars' if we are the main thread * (including non-threaded programs), or if an allocation * fails. */ if (thr_main()) - return (&nc_error); + return (&nc_vars); if (thr_once(&nc_once, nc_key_init) !=3D 0 || = nc_key_error !=3D 0) - return (&nc_error); - if ((nc_addr =3D (int *)thr_getspecific(nc_key)) =3D=3D = NULL) { - nc_addr =3D (int *)malloc(sizeof (int)); + return (&nc_vars); + if ((nc_addr =3D (struct nc_thread_vars *)thr_getspecific(nc_key)) + =3D=3D NULL) { + nc_addr =3D (struct nc_thread_vars *) + calloc(1, sizeof(struct nc_thread_vars)); if (thr_setspecific(nc_key, (void *) nc_addr) !=3D 0) { if (nc_addr) =20 free(nc_addr); - return (&nc_error); + return (&nc_vars); } - *nc_addr =3D 0; } return (nc_addr); } -#define nc_error (*(__nc_error())) +#define nc_error (*(&(__nc_thread()->nc_error))) +#define nc_file (*(&(__nc_thread()->nc_file))) +#define ni (*(&(__nc_thread()->nc_ni))) /* * A call to setnetconfig() establishes a /etc/netconfig "session". A session * "handle" is returned on a successful call. At the start of a session (after @@ -209,23 +211,14 @@ setnetconfig() * For multiple calls, i.e. nc_file is not NULL, we just return the * handle without reopening the netconfig db. */ - mutex_lock(&ni_lock); ni.ref++; - mutex_unlock(&ni_lock); - - mutex_lock(&nc_file_lock); if ((nc_file !=3D NULL) || (nc_file =3D fopen(NETCONFIG, "r")) !=3D = NULL) { nc_vars->valid =3D NC_VALID; nc_vars->flag =3D 0; nc_vars->nc_configs =3D ni.head; - mutex_unlock(&nc_file_lock); return ((void *)nc_vars); } - mutex_unlock(&nc_file_lock); - - mutex_lock(&ni_lock); ni.ref--; - mutex_unlock(&ni_lock); nc_error =3D NC_NONETCONFIG; free(nc_vars); @@ -254,13 +247,10 @@ void *handlep; /* * Verify that handle is valid */ - mutex_lock(&nc_file_lock); if (ncp =3D=3D NULL || nc_file =3D=3D NULL) { nc_error =3D NC_NOTINIT; - mutex_unlock(&nc_file_lock); return (NULL); } - mutex_unlock(&nc_file_lock); switch (ncp->valid) { case NC_VALID: @@ -274,9 +264,7 @@ void *handlep; */ if (ncp->flag =3D=3D 0) { /* first time */ ncp->flag =3D 1; - mutex_lock(&ni_lock); ncp->nc_configs =3D ni.head; - mutex_unlock(&ni_lock); if (ncp->nc_configs !=3D NULL) /* entry already = exist */ return(ncp->nc_configs->ncp); } @@ -289,12 +277,9 @@ void *handlep; * If we cannot find the entry in the list and is end of file, * we give up. */ - mutex_lock(&ni_lock); if (ni.eof =3D=3D 1) { - mutex_unlock(&ni_lock); return(NULL); } - mutex_unlock(&ni_lock); break; default: @@ -316,18 +301,13 @@ void *handlep; /* * Read a line from netconfig file. */ - mutex_lock(&nc_file_lock); do { if (fgets(stringp, MAXNETCONFIGLINE, nc_file) =3D=3D = NULL) { free(stringp); - mutex_lock(&ni_lock); ni.eof =3D 1; - mutex_unlock(&ni_lock); - mutex_unlock(&nc_file_lock); return (NULL); } } while (*stringp =3D=3D '#'); - mutex_unlock(&nc_file_lock); list =3D (struct netconfig_list *) malloc(sizeof (struct netconfig_list)); if (list =3D=3D NULL) { @@ -357,7 +337,6 @@ void *handlep; * Reposition the current pointer of the handle to the last entry * in the list. */ - mutex_lock(&ni_lock); if (ni.head =3D=3D NULL) { /* first entry */ ni.head =3D ni.tail =3D list; } @@ -367,7 +346,6 @@ void *handlep; } ncp->nc_configs =3D ni.tail; result =3D ni.tail->ncp; - mutex_unlock(&ni_lock); return(result); } } @@ -402,9 +380,7 @@ void *handlep; nc_handlep->valid =3D NC_INVALID; nc_handlep->flag =3D 0; nc_handlep->nc_configs =3D NULL; - mutex_lock(&ni_lock); if (--ni.ref > 0) { - mutex_unlock(&ni_lock); free(nc_handlep); return(0); } @@ -417,7 +393,6 @@ void *handlep; ni.eof =3D ni.ref =3D 0; ni.head =3D NULL; ni.tail =3D NULL; - mutex_unlock(&ni_lock); while (q !=3D NULL) { p =3D q->next; @@ -429,10 +404,8 @@ void *handlep; } free(nc_handlep); - mutex_lock(&nc_file_lock); fclose(nc_file); nc_file =3D NULL; - mutex_unlock(&nc_file_lock); return (0); } @@ -481,21 +454,16 @@ getnetconfigent(netid) * If all the netconfig db has been read and placed into the list and * there is no match for the netid, return NULL. */ - mutex_lock(&ni_lock); if (ni.head !=3D NULL) { for (list =3D ni.head; list; list =3D list->next) { if (strcmp(list->ncp->nc_netid, netid) =3D=3D 0) { - mutex_unlock(&ni_lock); return(dup_ncp(list->ncp)); } } if (ni.eof =3D=3D 1) { /* that's all the entries */ - mutex_unlock(&ni_lock); return(NULL); } } - mutex_unlock(&ni_lock); - if ((file =3D fopen(NETCONFIG, "r")) =3D=3D NULL) { nc_error =3D NC_NONETCONFIG; --=20 1.7.9.5 =20 From owner-freebsd-threads@FreeBSD.ORG Fri Aug 24 20:14:21 2012 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 6CCE71065670 for ; Fri, 24 Aug 2012 20:14:21 +0000 (UTC) (envelope-from bright@elvis.mu.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id 547C98FC0C for ; Fri, 24 Aug 2012 20:14:21 +0000 (UTC) Received: by elvis.mu.org (Postfix, from userid 1192) id 4AF811A3D51; Fri, 24 Aug 2012 13:07:14 -0700 (PDT) Date: Fri, 24 Aug 2012 13:07:14 -0700 From: Alfred Perlstein To: Steven Haber Message-ID: <20120824200714.GM68801@elvis.mu.org> References: <56CE86D6660FF84498426FA7A33170B40413DF8E@seaxch01.desktop.isilon.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56CE86D6660FF84498426FA7A33170B40413DF8E@seaxch01.desktop.isilon.com> User-Agent: Mutt/1.4.2.3i Cc: snnn119@gmail.com, freebsd-threads@freebsd.org Subject: Re: netconfig is not threadsafe X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Aug 2012 20:14:21 -0000 I suppose it can be fixed like this and the idea is pretty sound, however I was wondering if you can you look at how Linux/Solaris/OSX handle this? I would think there would be some form of "_r" routines that they would use instead. Or maybe thier libc avoids calling these routines somehow? That said this is good work and should go in barring other implementations that may make more sense. -Alfred * Steven Haber [120824 13:00] wrote: > netconfig (libc routines for reading the netconfig file) is currently > not threadsafe. It is a transactional API set, that is, you call > setnetconfig() to open the file, read from the file with getnetconfig(), > and close/cleanup with endnetconfig(). The problem is that the order > that these routines are called in is important, but the state of the > current transaction is stored in a per-process static, so only a single > thread can do a transaction at one time. A patch from five years ago > attempted to fix this: > > > > http://lists.freebsd.org/mailman/confirm/freebsd-threads/292f06b38808587 > 41c9034fb4173a14740c63a9e > > > > This patch does not solve the problem. It adds protection for the > statics so that concurrent calls into set/get/endnetgrent() don't trash > the statics. However, there is no use case where you would want two > threads accessing any of these statics concurrently. To solve this > problem, I have stored the FILE and netconfig_info statics on a > per-thread basis using the pthreads key setup that was already in place > for the error code. Now threads have their own set of stored state data, > so no statics are necessary. I removed the original locking that had no > effect. > > > > My repro is to spawn 32 threads that all create an RPC client > simultaneously using libc's clnt_create(), which calls into netconfig. > After patching, I no longer see the issue. Here is the stack when a > thread collision occurs (same as in the original bug from 2007): > > > > #0 fclose (fp=0x0) at > /root/Source/onefs/git/src/lib/libc/stdio/fclose.c:52 > > #1 0x0000000802c83a79 in endnetconfig (handlep=0x80b1030a0) at > /root/Source/onefs/git/src/lib/libc/rpc/getnetconfig.c:433 > > #2 0x0000000802c811d0 in __rpc_endconf (vhandle=0x80b103090) at > /root/Source/onefs/git/src/lib/libc/rpc/rpc_generic.c:445 > > #3 0x0000000802c7331a in clnt_create_timed (hostname=0x804d7cd9b > "tgroup %s", prog=134420864, vers=1, netclass=Variable "netclass" is not > available. > > ) at /root/Source/onefs/git/src/lib/libc/rpc/clnt_generic.c:271 > > > > This issue is present in FreeBSD head, though I'm reproducing on 7.1. > > > > Does the patch make sense? Should I open a bug on this somehow? I'm > relatively new to contributing to FreeBSD. > > > > Thanks, > > Steven > > > > > > >From f3639afbe7993f3c699f23f4d77749ea01e8e8c8 Mon Sep 17 00:00:00 2001 > > From: Steven Haber > > Date: Thu, 23 Aug 2012 17:45:51 -0700 > > Subject: [PATCH] Fix for bug 95757. Make netconfig threadsafe so we can > make > > lots of RPC clients concurrently. The original locking had > > no effect; there is no point in protecting the static > > members since you should never have two threads in the same > > get/set/endnetconfig. The point of these functions is to be > > called in sequence as a transaction. You can't have > > multiple threads calling them if the state is stored as a > > process global. This fix moves the static vars to thread- > > specific storage so that each thread can have its own > > transaction, and removes the exiting locking. > > --- > > src/lib/libc/rpc/getnetconfig.c | 74 > +++++++++++---------------------------- > > 1 file changed, 21 insertions(+), 53 deletions(-) > > > > diff --git a/src/lib/libc/rpc/getnetconfig.c > b/src/lib/libc/rpc/getnetconfig.c > > index 0f7168f..2957d14 100644 > > --- a/src/lib/libc/rpc/getnetconfig.c > > +++ b/src/lib/libc/rpc/getnetconfig.c > > @@ -119,22 +119,21 @@ struct netconfig_vars { > > struct netconfig_list *nc_configs; /* pointer to the current > netconfig entry */ > > }; > > +struct nc_thread_vars { > > + int nc_error; > > + FILE *nc_file; > > + struct netconfig_info nc_ni; > > +}; > > + > > #define NC_VALID 0xfeed > > #define NC_STORAGE 0xf00d > > #define NC_INVALID 0 > > > > -static int *__nc_error(void); > > static int parse_ncp(char *, struct netconfig *); > > static struct netconfig *dup_ncp(struct netconfig *); > > > > -static FILE *nc_file; /* for netconfig db */ > > -static mutex_t nc_file_lock = MUTEX_INITIALIZER; > > - > > -static struct netconfig_info ni = { 0, 0, NULL, NULL}; > > -static mutex_t ni_lock = MUTEX_INITIALIZER; > > - > > static thread_key_t nc_key; > > static once_t nc_once = ONCE_INITIALIZER; > > static int nc_key_error; > > @@ -148,34 +147,37 @@ nc_key_init(void) > > #define MAXNETCONFIGLINE 1000 > > -static int * > > -__nc_error() > > +static struct nc_thread_vars * > > +__nc_thread() > > { > > - static int nc_error = 0; > > - int *nc_addr; > > + static struct nc_thread_vars nc_vars = {0, NULL, {0, 0, > NULL, NULL}}; > > + struct nc_thread_vars *nc_addr; > > /* > > - * Use the static `nc_error' if we are the main thread > > + * Use the static `nc_vars' if we are the main thread > > * (including non-threaded programs), or if an allocation > > * fails. > > */ > > if (thr_main()) > > - return (&nc_error); > > + return (&nc_vars); > > if (thr_once(&nc_once, nc_key_init) != 0 || nc_key_error > != 0) > > - return (&nc_error); > > - if ((nc_addr = (int *)thr_getspecific(nc_key)) == NULL) { > > - nc_addr = (int *)malloc(sizeof (int)); > > + return (&nc_vars); > > + if ((nc_addr = (struct nc_thread_vars > *)thr_getspecific(nc_key)) > > + == NULL) { > > + nc_addr = (struct nc_thread_vars *) > > + calloc(1, sizeof(struct > nc_thread_vars)); > > if (thr_setspecific(nc_key, (void *) > nc_addr) != 0) { > > if (nc_addr) > > > free(nc_addr); > > - return (&nc_error); > > + return (&nc_vars); > > } > > - *nc_addr = 0; > > } > > return (nc_addr); > > } > > -#define nc_error (*(__nc_error())) > > +#define nc_error (*(&(__nc_thread()->nc_error))) > > +#define nc_file (*(&(__nc_thread()->nc_file))) > > +#define ni (*(&(__nc_thread()->nc_ni))) > > /* > > * A call to setnetconfig() establishes a /etc/netconfig "session". A > session > > * "handle" is returned on a successful call. At the start of a > session (after > > @@ -209,23 +211,14 @@ setnetconfig() > > * For multiple calls, i.e. nc_file is not NULL, we just return the > > * handle without reopening the netconfig db. > > */ > > - mutex_lock(&ni_lock); > > ni.ref++; > > - mutex_unlock(&ni_lock); > > - > > - mutex_lock(&nc_file_lock); > > if ((nc_file != NULL) || (nc_file = fopen(NETCONFIG, "r")) != NULL) > { > > nc_vars->valid = NC_VALID; > > nc_vars->flag = 0; > > nc_vars->nc_configs = ni.head; > > - mutex_unlock(&nc_file_lock); > > return ((void *)nc_vars); > > } > > - mutex_unlock(&nc_file_lock); > > - > > - mutex_lock(&ni_lock); > > ni.ref--; > > - mutex_unlock(&ni_lock); > > nc_error = NC_NONETCONFIG; > > free(nc_vars); > > @@ -254,13 +247,10 @@ void *handlep; > > /* > > * Verify that handle is valid > > */ > > - mutex_lock(&nc_file_lock); > > if (ncp == NULL || nc_file == NULL) { > > nc_error = NC_NOTINIT; > > - mutex_unlock(&nc_file_lock); > > return (NULL); > > } > > - mutex_unlock(&nc_file_lock); > > switch (ncp->valid) { > > case NC_VALID: > > @@ -274,9 +264,7 @@ void *handlep; > > */ > > if (ncp->flag == 0) { /* first time */ > > ncp->flag = 1; > > - mutex_lock(&ni_lock); > > ncp->nc_configs = ni.head; > > - mutex_unlock(&ni_lock); > > if (ncp->nc_configs != NULL) /* entry already exist > */ > > return(ncp->nc_configs->ncp); > > } > > @@ -289,12 +277,9 @@ void *handlep; > > * If we cannot find the entry in the list and is end of > file, > > * we give up. > > */ > > - mutex_lock(&ni_lock); > > if (ni.eof == 1) { > > - mutex_unlock(&ni_lock); > > return(NULL); > > } > > - mutex_unlock(&ni_lock); > > break; > > default: > > @@ -316,18 +301,13 @@ void *handlep; > > /* > > * Read a line from netconfig file. > > */ > > - mutex_lock(&nc_file_lock); > > do { > > if (fgets(stringp, MAXNETCONFIGLINE, nc_file) == NULL) { > > free(stringp); > > - mutex_lock(&ni_lock); > > ni.eof = 1; > > - mutex_unlock(&ni_lock); > > - mutex_unlock(&nc_file_lock); > > return (NULL); > > } > > } while (*stringp == '#'); > > - mutex_unlock(&nc_file_lock); > > list = (struct netconfig_list *) malloc(sizeof (struct > netconfig_list)); > > if (list == NULL) { > > @@ -357,7 +337,6 @@ void *handlep; > > * Reposition the current pointer of the handle to the > last entry > > * in the list. > > */ > > - mutex_lock(&ni_lock); > > if (ni.head == NULL) { /* first entry */ > > ni.head = ni.tail = list; > > } > > @@ -367,7 +346,6 @@ void *handlep; > > } > > ncp->nc_configs = ni.tail; > > result = ni.tail->ncp; > > - mutex_unlock(&ni_lock); > > return(result); > > } > > } > > @@ -402,9 +380,7 @@ void *handlep; > > nc_handlep->valid = NC_INVALID; > > nc_handlep->flag = 0; > > nc_handlep->nc_configs = NULL; > > - mutex_lock(&ni_lock); > > if (--ni.ref > 0) { > > - mutex_unlock(&ni_lock); > > free(nc_handlep); > > return(0); > > } > > @@ -417,7 +393,6 @@ void *handlep; > > ni.eof = ni.ref = 0; > > ni.head = NULL; > > ni.tail = NULL; > > - mutex_unlock(&ni_lock); > > while (q != NULL) { > > p = q->next; > > @@ -429,10 +404,8 @@ void *handlep; > > } > > free(nc_handlep); > > - mutex_lock(&nc_file_lock); > > fclose(nc_file); > > nc_file = NULL; > > - mutex_unlock(&nc_file_lock); > > return (0); > > } > > @@ -481,21 +454,16 @@ getnetconfigent(netid) > > * If all the netconfig db has been read and placed into the list > and > > * there is no match for the netid, return NULL. > > */ > > - mutex_lock(&ni_lock); > > if (ni.head != NULL) { > > for (list = ni.head; list; list = list->next) { > > if (strcmp(list->ncp->nc_netid, netid) == 0) { > > - mutex_unlock(&ni_lock); > > return(dup_ncp(list->ncp)); > > } > > } > > if (ni.eof == 1) { /* that's all the entries */ > > - mutex_unlock(&ni_lock); > > return(NULL); > > } > > } > > - mutex_unlock(&ni_lock); > > - > > if ((file = fopen(NETCONFIG, "r")) == NULL) { > > nc_error = NC_NONETCONFIG; > > -- > > 1.7.9.5 > > > > _______________________________________________ > freebsd-threads@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-threads > To unsubscribe, send any mail to "freebsd-threads-unsubscribe@freebsd.org" -- - Alfred Perlstein .- VMOA #5191, 03 vmax, 92 gs500, 85 ch250, 07 zx10 .- FreeBSD committer From owner-freebsd-threads@FreeBSD.ORG Fri Aug 24 20:17:46 2012 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 28E48106564A for ; Fri, 24 Aug 2012 20:17:46 +0000 (UTC) (envelope-from bright@elvis.mu.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id 10B088FC17 for ; Fri, 24 Aug 2012 20:17:45 +0000 (UTC) Received: by elvis.mu.org (Postfix, from userid 1192) id BDB111A3D6C; Fri, 24 Aug 2012 13:17:45 -0700 (PDT) Date: Fri, 24 Aug 2012 13:17:45 -0700 From: Alfred Perlstein To: Steven Haber Message-ID: <20120824201745.GO68801@elvis.mu.org> References: <56CE86D6660FF84498426FA7A33170B40413DF8E@seaxch01.desktop.isilon.com> <20120824200714.GM68801@elvis.mu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120824200714.GM68801@elvis.mu.org> User-Agent: Mutt/1.4.2.3i Cc: snnn119@gmail.com, freebsd-threads@freebsd.org Subject: Re: netconfig is not threadsafe X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Aug 2012 20:17:46 -0000 I did a bit of research and this manual page: http://www.unix.com/man-page/All/3n/setnetconfig/ seems to indicate that this is the best that is "expected" for thread safety: MULTITHREAD USAGE Thread Safe: Yes Cancel Safe: Yes Fork Safe: No Async-cancel Safe: No Async-signal Safe: No These functions can be called safely in a multithreaded environment. They may be cancellation points in that they call functions that are cancel points. In a multithreaded environment, these functions are not safe to be called by a child process after and before These functions should not be called by a multithreaded application that supports asynchronous cancellation or asynchronous signals. So this is awesome. Thank you. -Alfred * Alfred Perlstein [120824 13:14] wrote: > I suppose it can be fixed like this and the idea is pretty sound, > however I was wondering if you can you look at how Linux/Solaris/OSX > handle this? > > I would think there would be some form of "_r" routines that they > would use instead. Or maybe thier libc avoids calling these routines > somehow? > > That said this is good work and should go in barring other > implementations that may make more sense. > > -Alfred > > * Steven Haber [120824 13:00] wrote: > > netconfig (libc routines for reading the netconfig file) is currently > > not threadsafe. It is a transactional API set, that is, you call > > setnetconfig() to open the file, read from the file with getnetconfig(), > > and close/cleanup with endnetconfig(). The problem is that the order > > that these routines are called in is important, but the state of the > > current transaction is stored in a per-process static, so only a single > > thread can do a transaction at one time. A patch from five years ago > > attempted to fix this: > > > > > > > > http://lists.freebsd.org/mailman/confirm/freebsd-threads/292f06b38808587 > > 41c9034fb4173a14740c63a9e > > > > > > > > This patch does not solve the problem. It adds protection for the > > statics so that concurrent calls into set/get/endnetgrent() don't trash > > the statics. However, there is no use case where you would want two > > threads accessing any of these statics concurrently. To solve this > > problem, I have stored the FILE and netconfig_info statics on a > > per-thread basis using the pthreads key setup that was already in place > > for the error code. Now threads have their own set of stored state data, > > so no statics are necessary. I removed the original locking that had no > > effect. > > > > > > > > My repro is to spawn 32 threads that all create an RPC client > > simultaneously using libc's clnt_create(), which calls into netconfig. > > After patching, I no longer see the issue. Here is the stack when a > > thread collision occurs (same as in the original bug from 2007): > > > > > > > > #0 fclose (fp=0x0) at > > /root/Source/onefs/git/src/lib/libc/stdio/fclose.c:52 > > > > #1 0x0000000802c83a79 in endnetconfig (handlep=0x80b1030a0) at > > /root/Source/onefs/git/src/lib/libc/rpc/getnetconfig.c:433 > > > > #2 0x0000000802c811d0 in __rpc_endconf (vhandle=0x80b103090) at > > /root/Source/onefs/git/src/lib/libc/rpc/rpc_generic.c:445 > > > > #3 0x0000000802c7331a in clnt_create_timed (hostname=0x804d7cd9b > > "tgroup %s", prog=134420864, vers=1, netclass=Variable "netclass" is not > > available. > > > > ) at /root/Source/onefs/git/src/lib/libc/rpc/clnt_generic.c:271 > > > > > > > > This issue is present in FreeBSD head, though I'm reproducing on 7.1. > > > > > > > > Does the patch make sense? Should I open a bug on this somehow? I'm > > relatively new to contributing to FreeBSD. > > > > > > > > Thanks, > > > > Steven > > > > > > > > > > > > >From f3639afbe7993f3c699f23f4d77749ea01e8e8c8 Mon Sep 17 00:00:00 2001 > > > > From: Steven Haber > > > > Date: Thu, 23 Aug 2012 17:45:51 -0700 > > > > Subject: [PATCH] Fix for bug 95757. Make netconfig threadsafe so we can > > make > > > > lots of RPC clients concurrently. The original locking had > > > > no effect; there is no point in protecting the static > > > > members since you should never have two threads in the same > > > > get/set/endnetconfig. The point of these functions is to be > > > > called in sequence as a transaction. You can't have > > > > multiple threads calling them if the state is stored as a > > > > process global. This fix moves the static vars to thread- > > > > specific storage so that each thread can have its own > > > > transaction, and removes the exiting locking. > > > > --- > > > > src/lib/libc/rpc/getnetconfig.c | 74 > > +++++++++++---------------------------- > > > > 1 file changed, 21 insertions(+), 53 deletions(-) > > > > > > > > diff --git a/src/lib/libc/rpc/getnetconfig.c > > b/src/lib/libc/rpc/getnetconfig.c > > > > index 0f7168f..2957d14 100644 > > > > --- a/src/lib/libc/rpc/getnetconfig.c > > > > +++ b/src/lib/libc/rpc/getnetconfig.c > > > > @@ -119,22 +119,21 @@ struct netconfig_vars { > > > > struct netconfig_list *nc_configs; /* pointer to the current > > netconfig entry */ > > > > }; > > > > +struct nc_thread_vars { > > > > + int nc_error; > > > > + FILE *nc_file; > > > > + struct netconfig_info nc_ni; > > > > +}; > > > > + > > > > #define NC_VALID 0xfeed > > > > #define NC_STORAGE 0xf00d > > > > #define NC_INVALID 0 > > > > > > > > -static int *__nc_error(void); > > > > static int parse_ncp(char *, struct netconfig *); > > > > static struct netconfig *dup_ncp(struct netconfig *); > > > > > > > > -static FILE *nc_file; /* for netconfig db */ > > > > -static mutex_t nc_file_lock = MUTEX_INITIALIZER; > > > > - > > > > -static struct netconfig_info ni = { 0, 0, NULL, NULL}; > > > > -static mutex_t ni_lock = MUTEX_INITIALIZER; > > > > - > > > > static thread_key_t nc_key; > > > > static once_t nc_once = ONCE_INITIALIZER; > > > > static int nc_key_error; > > > > @@ -148,34 +147,37 @@ nc_key_init(void) > > > > #define MAXNETCONFIGLINE 1000 > > > > -static int * > > > > -__nc_error() > > > > +static struct nc_thread_vars * > > > > +__nc_thread() > > > > { > > > > - static int nc_error = 0; > > > > - int *nc_addr; > > > > + static struct nc_thread_vars nc_vars = {0, NULL, {0, 0, > > NULL, NULL}}; > > > > + struct nc_thread_vars *nc_addr; > > > > /* > > > > - * Use the static `nc_error' if we are the main thread > > > > + * Use the static `nc_vars' if we are the main thread > > > > * (including non-threaded programs), or if an allocation > > > > * fails. > > > > */ > > > > if (thr_main()) > > > > - return (&nc_error); > > > > + return (&nc_vars); > > > > if (thr_once(&nc_once, nc_key_init) != 0 || nc_key_error > > != 0) > > > > - return (&nc_error); > > > > - if ((nc_addr = (int *)thr_getspecific(nc_key)) == NULL) { > > > > - nc_addr = (int *)malloc(sizeof (int)); > > > > + return (&nc_vars); > > > > + if ((nc_addr = (struct nc_thread_vars > > *)thr_getspecific(nc_key)) > > > > + == NULL) { > > > > + nc_addr = (struct nc_thread_vars *) > > > > + calloc(1, sizeof(struct > > nc_thread_vars)); > > > > if (thr_setspecific(nc_key, (void *) > > nc_addr) != 0) { > > > > if (nc_addr) > > > > > > free(nc_addr); > > > > - return (&nc_error); > > > > + return (&nc_vars); > > > > } > > > > - *nc_addr = 0; > > > > } > > > > return (nc_addr); > > > > } > > > > -#define nc_error (*(__nc_error())) > > > > +#define nc_error (*(&(__nc_thread()->nc_error))) > > > > +#define nc_file (*(&(__nc_thread()->nc_file))) > > > > +#define ni (*(&(__nc_thread()->nc_ni))) > > > > /* > > > > * A call to setnetconfig() establishes a /etc/netconfig "session". A > > session > > > > * "handle" is returned on a successful call. At the start of a > > session (after > > > > @@ -209,23 +211,14 @@ setnetconfig() > > > > * For multiple calls, i.e. nc_file is not NULL, we just return the > > > > * handle without reopening the netconfig db. > > > > */ > > > > - mutex_lock(&ni_lock); > > > > ni.ref++; > > > > - mutex_unlock(&ni_lock); > > > > - > > > > - mutex_lock(&nc_file_lock); > > > > if ((nc_file != NULL) || (nc_file = fopen(NETCONFIG, "r")) != NULL) > > { > > > > nc_vars->valid = NC_VALID; > > > > nc_vars->flag = 0; > > > > nc_vars->nc_configs = ni.head; > > > > - mutex_unlock(&nc_file_lock); > > > > return ((void *)nc_vars); > > > > } > > > > - mutex_unlock(&nc_file_lock); > > > > - > > > > - mutex_lock(&ni_lock); > > > > ni.ref--; > > > > - mutex_unlock(&ni_lock); > > > > nc_error = NC_NONETCONFIG; > > > > free(nc_vars); > > > > @@ -254,13 +247,10 @@ void *handlep; > > > > /* > > > > * Verify that handle is valid > > > > */ > > > > - mutex_lock(&nc_file_lock); > > > > if (ncp == NULL || nc_file == NULL) { > > > > nc_error = NC_NOTINIT; > > > > - mutex_unlock(&nc_file_lock); > > > > return (NULL); > > > > } > > > > - mutex_unlock(&nc_file_lock); > > > > switch (ncp->valid) { > > > > case NC_VALID: > > > > @@ -274,9 +264,7 @@ void *handlep; > > > > */ > > > > if (ncp->flag == 0) { /* first time */ > > > > ncp->flag = 1; > > > > - mutex_lock(&ni_lock); > > > > ncp->nc_configs = ni.head; > > > > - mutex_unlock(&ni_lock); > > > > if (ncp->nc_configs != NULL) /* entry already exist > > */ > > > > return(ncp->nc_configs->ncp); > > > > } > > > > @@ -289,12 +277,9 @@ void *handlep; > > > > * If we cannot find the entry in the list and is end of > > file, > > > > * we give up. > > > > */ > > > > - mutex_lock(&ni_lock); > > > > if (ni.eof == 1) { > > > > - mutex_unlock(&ni_lock); > > > > return(NULL); > > > > } > > > > - mutex_unlock(&ni_lock); > > > > break; > > > > default: > > > > @@ -316,18 +301,13 @@ void *handlep; > > > > /* > > > > * Read a line from netconfig file. > > > > */ > > > > - mutex_lock(&nc_file_lock); > > > > do { > > > > if (fgets(stringp, MAXNETCONFIGLINE, nc_file) == NULL) { > > > > free(stringp); > > > > - mutex_lock(&ni_lock); > > > > ni.eof = 1; > > > > - mutex_unlock(&ni_lock); > > > > - mutex_unlock(&nc_file_lock); > > > > return (NULL); > > > > } > > > > } while (*stringp == '#'); > > > > - mutex_unlock(&nc_file_lock); > > > > list = (struct netconfig_list *) malloc(sizeof (struct > > netconfig_list)); > > > > if (list == NULL) { > > > > @@ -357,7 +337,6 @@ void *handlep; > > > > * Reposition the current pointer of the handle to the > > last entry > > > > * in the list. > > > > */ > > > > - mutex_lock(&ni_lock); > > > > if (ni.head == NULL) { /* first entry */ > > > > ni.head = ni.tail = list; > > > > } > > > > @@ -367,7 +346,6 @@ void *handlep; > > > > } > > > > ncp->nc_configs = ni.tail; > > > > result = ni.tail->ncp; > > > > - mutex_unlock(&ni_lock); > > > > return(result); > > > > } > > > > } > > > > @@ -402,9 +380,7 @@ void *handlep; > > > > nc_handlep->valid = NC_INVALID; > > > > nc_handlep->flag = 0; > > > > nc_handlep->nc_configs = NULL; > > > > - mutex_lock(&ni_lock); > > > > if (--ni.ref > 0) { > > > > - mutex_unlock(&ni_lock); > > > > free(nc_handlep); > > > > return(0); > > > > } > > > > @@ -417,7 +393,6 @@ void *handlep; > > > > ni.eof = ni.ref = 0; > > > > ni.head = NULL; > > > > ni.tail = NULL; > > > > - mutex_unlock(&ni_lock); > > > > while (q != NULL) { > > > > p = q->next; > > > > @@ -429,10 +404,8 @@ void *handlep; > > > > } > > > > free(nc_handlep); > > > > - mutex_lock(&nc_file_lock); > > > > fclose(nc_file); > > > > nc_file = NULL; > > > > - mutex_unlock(&nc_file_lock); > > > > return (0); > > > > } > > > > @@ -481,21 +454,16 @@ getnetconfigent(netid) > > > > * If all the netconfig db has been read and placed into the list > > and > > > > * there is no match for the netid, return NULL. > > > > */ > > > > - mutex_lock(&ni_lock); > > > > if (ni.head != NULL) { > > > > for (list = ni.head; list; list = list->next) { > > > > if (strcmp(list->ncp->nc_netid, netid) == 0) { > > > > - mutex_unlock(&ni_lock); > > > > return(dup_ncp(list->ncp)); > > > > } > > > > } > > > > if (ni.eof == 1) { /* that's all the entries */ > > > > - mutex_unlock(&ni_lock); > > > > return(NULL); > > > > } > > > > } > > > > - mutex_unlock(&ni_lock); > > > > - > > > > if ((file = fopen(NETCONFIG, "r")) == NULL) { > > > > nc_error = NC_NONETCONFIG; > > > > -- > > > > 1.7.9.5 > > > > > > > > _______________________________________________ > > freebsd-threads@freebsd.org mailing list > > http://lists.freebsd.org/mailman/listinfo/freebsd-threads > > To unsubscribe, send any mail to "freebsd-threads-unsubscribe@freebsd.org" > > -- > - Alfred Perlstein > .- VMOA #5191, 03 vmax, 92 gs500, 85 ch250, 07 zx10 > .- FreeBSD committer > _______________________________________________ > freebsd-threads@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-threads > To unsubscribe, send any mail to "freebsd-threads-unsubscribe@freebsd.org" -- - Alfred Perlstein .- VMOA #5191, 03 vmax, 92 gs500, 85 ch250, 07 zx10 .- FreeBSD committer From owner-freebsd-threads@FreeBSD.ORG Fri Aug 24 22:12:54 2012 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 1C0E41065670; Fri, 24 Aug 2012 22:12:54 +0000 (UTC) (envelope-from steven.haber@isilon.com) Received: from mexforward.lss.emc.com (hop-nat-141.emc.com [168.159.213.141]) by mx1.freebsd.org (Postfix) with ESMTP id 538568FC15; Fri, 24 Aug 2012 22:12:53 +0000 (UTC) Received: from hop04-l1d11-si03.isus.emc.com (HOP04-L1D11-SI03.isus.emc.com [10.254.111.23]) by mexforward.lss.emc.com (Switch-3.4.3/Switch-3.4.3) with ESMTP id q7OMClJ5025160 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 24 Aug 2012 18:12:51 -0400 Received: from mailhub.lss.emc.com (mailhub.lss.emc.com [10.254.222.226]) by hop04-l1d11-si03.isus.emc.com (RSA Interceptor); Fri, 24 Aug 2012 18:12:38 -0400 Received: from seacasht02.desktop.isilon.com (seacasht02.isilon.com [137.69.159.81]) by mailhub.lss.emc.com (Switch-3.4.3/Switch-3.4.3) with ESMTP id q7OMCJlU018299; Fri, 24 Aug 2012 18:12:37 -0400 Received: from seaxch01.isilon.com (137.69.158.60) by SEACASHT02.desktop.isilon.com (137.69.159.81) with Microsoft SMTP Server id 14.2.298.4; Fri, 24 Aug 2012 15:08:08 -0700 Received: from [10.7.181.140] ([10.7.181.140]) by seaxch01.isilon.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 24 Aug 2012 15:08:02 -0700 Message-ID: <5037FAF5.5000305@isilon.com> Date: Fri, 24 Aug 2012 15:06:45 -0700 From: Steven Haber User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120714 Thunderbird/14.0 MIME-Version: 1.0 To: Alfred Perlstein References: <56CE86D6660FF84498426FA7A33170B40413DF8E@seaxch01.desktop.isilon.com> <20120824200714.GM68801@elvis.mu.org> <20120824201745.GO68801@elvis.mu.org> In-Reply-To: <20120824201745.GO68801@elvis.mu.org> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-OriginalArrivalTime: 24 Aug 2012 22:08:02.0310 (UTC) FILETIME=[EDD41A60:01CD8244] X-EMM-MHVC: 1 Cc: snnn119@gmail.com, freebsd-threads@freebsd.org Subject: Re: netconfig is not threadsafe X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Aug 2012 22:12:54 -0000 Yeah, I definitely think it should support threading. It looks like we only use netconfig a few places: RPC, and some sbin services like mountd, yp, and keyserv. So we could just serialize correctly in RPC/mountd/etc., but I think it's better to protect the whole thing. Linux (Ubuntu) has a FreeBSD man page for netconfig with no mention of _r routines; the APIs look pretty much like they do on FreeBSD. I can't seem to find the source in my libc, however. For netgrent (similar to netconfig) there is getnetgrent_r(), a GNU-specific call, which just stores retrieved strings in a buffer you pass in, but near as I can tell the calls aren't actually thread safe: http://sourceware.org/git/?p=glibc.git;a=blob;f=inet/getnetgrent_r.c;h=8e69ec7b1135dc886a1cf0730001fd709a701b87;hb=HEAD All the data is just stored in a static, and calls to set/get/endnetgrent are serialized by a lock, but you could call them out of order with multiple threads. In Solaris 10, you get a handle back from setnetconfig() that is unique to your session. This implementation sounds fully threadsafe. Haven't tried it. On OSX 10.7.2 the calls look the same as FreeBSD (no handle) and it does not mention whether or not it is threadsafe, though in BUGS it mentions: "The function getnetgrent() returns pointers to dynamically allocated data areas that are freed when the function endnetgrent() is called." I haven't tried threading it on OSX, either. I'm pasting a better version of the patch since Outlook destroyed my last one. Steven From f3639afbe7993f3c699f23f4d77749ea01e8e8c8 Mon Sep 17 00:00:00 2001 From: Steven Haber Date: Thu, 23 Aug 2012 17:45:51 -0700 Subject: [PATCH] Fix for bug 95757. Make netconfig threadsafe so we can make lots of RPC clients concurrently. The original locking had no effect; there is no point in protecting the static members since you should never have two threads in the same get/set/endnetconfig. The point of these functions is to be called in sequence as a transaction. You can't have multiple threads calling them if the state is stored as a process global. This fix moves the static vars to thread- specific storage so that each thread can have its own transaction, and removes the existing locking. --- src/lib/libc/rpc/getnetconfig.c | 74 +++++++++++---------------------------- 1 file changed, 21 insertions(+), 53 deletions(-) diff --git a/src/lib/libc/rpc/getnetconfig.c b/src/lib/libc/rpc/getnetconfig.c index 0f7168f..2957d14 100644 --- a/src/lib/libc/rpc/getnetconfig.c +++ b/src/lib/libc/rpc/getnetconfig.c @@ -119,22 +119,21 @@ struct netconfig_vars { struct netconfig_list *nc_configs; /* pointer to the current netconfig entry */ }; +struct nc_thread_vars { + int nc_error; + FILE *nc_file; + struct netconfig_info nc_ni; +}; + #define NC_VALID 0xfeed #define NC_STORAGE 0xf00d #define NC_INVALID 0 -static int *__nc_error(void); static int parse_ncp(char *, struct netconfig *); static struct netconfig *dup_ncp(struct netconfig *); -static FILE *nc_file; /* for netconfig db */ -static mutex_t nc_file_lock = MUTEX_INITIALIZER; - -static struct netconfig_info ni = { 0, 0, NULL, NULL}; -static mutex_t ni_lock = MUTEX_INITIALIZER; - static thread_key_t nc_key; static once_t nc_once = ONCE_INITIALIZER; static int nc_key_error; @@ -148,34 +147,37 @@ nc_key_init(void) #define MAXNETCONFIGLINE 1000 -static int * -__nc_error() +static struct nc_thread_vars * +__nc_thread() { - static int nc_error = 0; - int *nc_addr; + static struct nc_thread_vars nc_vars = {0, NULL, {0, 0, NULL, NULL}}; + struct nc_thread_vars *nc_addr; /* - * Use the static `nc_error' if we are the main thread + * Use the static `nc_vars' if we are the main thread * (including non-threaded programs), or if an allocation * fails. */ if (thr_main()) - return (&nc_error); + return (&nc_vars); if (thr_once(&nc_once, nc_key_init) != 0 || nc_key_error != 0) - return (&nc_error); - if ((nc_addr = (int *)thr_getspecific(nc_key)) == NULL) { - nc_addr = (int *)malloc(sizeof (int)); + return (&nc_vars); + if ((nc_addr = (struct nc_thread_vars *)thr_getspecific(nc_key)) + == NULL) { + nc_addr = (struct nc_thread_vars *) + calloc(1, sizeof(struct nc_thread_vars)); if (thr_setspecific(nc_key, (void *) nc_addr) != 0) { if (nc_addr) free(nc_addr); - return (&nc_error); + return (&nc_vars); } - *nc_addr = 0; } return (nc_addr); } -#define nc_error (*(__nc_error())) +#define nc_error (*(&(__nc_thread()->nc_error))) +#define nc_file (*(&(__nc_thread()->nc_file))) +#define ni (*(&(__nc_thread()->nc_ni))) /* * A call to setnetconfig() establishes a /etc/netconfig "session". A session * "handle" is returned on a successful call. At the start of a session (after @@ -209,23 +211,14 @@ setnetconfig() * For multiple calls, i.e. nc_file is not NULL, we just return the * handle without reopening the netconfig db. */ - mutex_lock(&ni_lock); ni.ref++; - mutex_unlock(&ni_lock); - - mutex_lock(&nc_file_lock); if ((nc_file != NULL) || (nc_file = fopen(NETCONFIG, "r")) != NULL) { nc_vars->valid = NC_VALID; nc_vars->flag = 0; nc_vars->nc_configs = ni.head; - mutex_unlock(&nc_file_lock); return ((void *)nc_vars); } - mutex_unlock(&nc_file_lock); - - mutex_lock(&ni_lock); ni.ref--; - mutex_unlock(&ni_lock); nc_error = NC_NONETCONFIG; free(nc_vars); @@ -254,13 +247,10 @@ void *handlep; /* * Verify that handle is valid */ - mutex_lock(&nc_file_lock); if (ncp == NULL || nc_file == NULL) { nc_error = NC_NOTINIT; - mutex_unlock(&nc_file_lock); return (NULL); } - mutex_unlock(&nc_file_lock); switch (ncp->valid) { case NC_VALID: @@ -274,9 +264,7 @@ void *handlep; */ if (ncp->flag == 0) { /* first time */ ncp->flag = 1; - mutex_lock(&ni_lock); ncp->nc_configs = ni.head; - mutex_unlock(&ni_lock); if (ncp->nc_configs != NULL) /* entry already exist */ return(ncp->nc_configs->ncp); } @@ -289,12 +277,9 @@ void *handlep; * If we cannot find the entry in the list and is end of file, * we give up. */ - mutex_lock(&ni_lock); if (ni.eof == 1) { - mutex_unlock(&ni_lock); return(NULL); } - mutex_unlock(&ni_lock); break; default: @@ -316,18 +301,13 @@ void *handlep; /* * Read a line from netconfig file. */ - mutex_lock(&nc_file_lock); do { if (fgets(stringp, MAXNETCONFIGLINE, nc_file) == NULL) { free(stringp); - mutex_lock(&ni_lock); ni.eof = 1; - mutex_unlock(&ni_lock); - mutex_unlock(&nc_file_lock); return (NULL); } } while (*stringp == '#'); - mutex_unlock(&nc_file_lock); list = (struct netconfig_list *) malloc(sizeof (struct netconfig_list)); if (list == NULL) { @@ -357,7 +337,6 @@ void *handlep; * Reposition the current pointer of the handle to the last entry * in the list. */ - mutex_lock(&ni_lock); if (ni.head == NULL) { /* first entry */ ni.head = ni.tail = list; } @@ -367,7 +346,6 @@ void *handlep; } ncp->nc_configs = ni.tail; result = ni.tail->ncp; - mutex_unlock(&ni_lock); return(result); } } @@ -402,9 +380,7 @@ void *handlep; nc_handlep->valid = NC_INVALID; nc_handlep->flag = 0; nc_handlep->nc_configs = NULL; - mutex_lock(&ni_lock); if (--ni.ref > 0) { - mutex_unlock(&ni_lock); free(nc_handlep); return(0); } @@ -417,7 +393,6 @@ void *handlep; ni.eof = ni.ref = 0; ni.head = NULL; ni.tail = NULL; - mutex_unlock(&ni_lock); while (q != NULL) { p = q->next; @@ -429,10 +404,8 @@ void *handlep; } free(nc_handlep); - mutex_lock(&nc_file_lock); fclose(nc_file); nc_file = NULL; - mutex_unlock(&nc_file_lock); return (0); } @@ -481,21 +454,16 @@ getnetconfigent(netid) * If all the netconfig db has been read and placed into the list and * there is no match for the netid, return NULL. */ - mutex_lock(&ni_lock); if (ni.head != NULL) { for (list = ni.head; list; list = list->next) { if (strcmp(list->ncp->nc_netid, netid) == 0) { - mutex_unlock(&ni_lock); return(dup_ncp(list->ncp)); } } if (ni.eof == 1) { /* that's all the entries */ - mutex_unlock(&ni_lock); return(NULL); } } - mutex_unlock(&ni_lock); - if ((file = fopen(NETCONFIG, "r")) == NULL) { nc_error = NC_NONETCONFIG; -- 1.7.9.5 On 08/24/2012 01:17 PM, Alfred Perlstein wrote: > I did a bit of research and this manual page: > http://www.unix.com/man-page/All/3n/setnetconfig/ > > seems to indicate that this is the best that is "expected" > for thread safety: > > MULTITHREAD USAGE > Thread Safe: Yes > Cancel Safe: Yes > Fork Safe: No > Async-cancel Safe: No > Async-signal Safe: No > > These functions can be called safely in a multithreaded environment. > They may be cancellation points in that they call functions that are > cancel points. > > In a multithreaded environment, these functions are not safe to be > called by a child process after and before These functions should not > be called by a multithreaded application that supports asynchronous > cancellation or asynchronous signals. > > So this is awesome. Thank you. > > -Alfred > > * Alfred Perlstein [120824 13:14] wrote: >> I suppose it can be fixed like this and the idea is pretty sound, >> however I was wondering if you can you look at how Linux/Solaris/OSX >> handle this? >> >> I would think there would be some form of "_r" routines that they >> would use instead. Or maybe thier libc avoids calling these routines >> somehow? >> >> That said this is good work and should go in barring other >> implementations that may make more sense. >> >> -Alfred >> >> * Steven Haber [120824 13:00] wrote: >>> netconfig (libc routines for reading the netconfig file) is currently >>> not threadsafe. It is a transactional API set, that is, you call >>> setnetconfig() to open the file, read from the file with getnetconfig(), >>> and close/cleanup with endnetconfig(). The problem is that the order >>> that these routines are called in is important, but the state of the >>> current transaction is stored in a per-process static, so only a single >>> thread can do a transaction at one time. A patch from five years ago >>> attempted to fix this: >>> >>> >>> >>> http://lists.freebsd.org/mailman/confirm/freebsd-threads/292f06b38808587 >>> 41c9034fb4173a14740c63a9e >>> >>> >>> >>> This patch does not solve the problem. It adds protection for the >>> statics so that concurrent calls into set/get/endnetgrent() don't trash >>> the statics. However, there is no use case where you would want two >>> threads accessing any of these statics concurrently. To solve this >>> problem, I have stored the FILE and netconfig_info statics on a >>> per-thread basis using the pthreads key setup that was already in place >>> for the error code. Now threads have their own set of stored state data, >>> so no statics are necessary. I removed the original locking that had no >>> effect. >>> >>> >>> >>> My repro is to spawn 32 threads that all create an RPC client >>> simultaneously using libc's clnt_create(), which calls into netconfig. >>> After patching, I no longer see the issue. Here is the stack when a >>> thread collision occurs (same as in the original bug from 2007): >>> >>> >>> >>> #0 fclose (fp=0x0) at >>> /root/Source/onefs/git/src/lib/libc/stdio/fclose.c:52 >>> >>> #1 0x0000000802c83a79 in endnetconfig (handlep=0x80b1030a0) at >>> /root/Source/onefs/git/src/lib/libc/rpc/getnetconfig.c:433 >>> >>> #2 0x0000000802c811d0 in __rpc_endconf (vhandle=0x80b103090) at >>> /root/Source/onefs/git/src/lib/libc/rpc/rpc_generic.c:445 >>> >>> #3 0x0000000802c7331a in clnt_create_timed (hostname=0x804d7cd9b >>> "tgroup %s", prog=134420864, vers=1, netclass=Variable "netclass" is not >>> available. >>> >>> ) at /root/Source/onefs/git/src/lib/libc/rpc/clnt_generic.c:271 >>> >>> >>> >>> This issue is present in FreeBSD head, though I'm reproducing on 7.1. >>> >>> >>> >>> Does the patch make sense? Should I open a bug on this somehow? I'm >>> relatively new to contributing to FreeBSD. >>> >>> >>> >>> Thanks, >>> >>> Steven >>> >>> >>> >>> >>> >>> >From f3639afbe7993f3c699f23f4d77749ea01e8e8c8 Mon Sep 17 00:00:00 2001 >>> >>> From: Steven Haber >>> >>> Date: Thu, 23 Aug 2012 17:45:51 -0700 >>> >>> Subject: [PATCH] Fix for bug 95757. Make netconfig threadsafe so we can >>> make >>> >>> lots of RPC clients concurrently. The original locking had >>> >>> no effect; there is no point in protecting the static >>> >>> members since you should never have two threads in the same >>> >>> get/set/endnetconfig. The point of these functions is to be >>> >>> called in sequence as a transaction. You can't have >>> >>> multiple threads calling them if the state is stored as a >>> >>> process global. This fix moves the static vars to thread- >>> >>> specific storage so that each thread can have its own >>> >>> transaction, and removes the exiting locking. >>> >>> --- >>> >>> src/lib/libc/rpc/getnetconfig.c | 74 >>> +++++++++++---------------------------- >>> >>> 1 file changed, 21 insertions(+), 53 deletions(-) >>> >>> >>> >>> diff --git a/src/lib/libc/rpc/getnetconfig.c >>> b/src/lib/libc/rpc/getnetconfig.c >>> >>> index 0f7168f..2957d14 100644 >>> >>> --- a/src/lib/libc/rpc/getnetconfig.c >>> >>> +++ b/src/lib/libc/rpc/getnetconfig.c >>> >>> @@ -119,22 +119,21 @@ struct netconfig_vars { >>> >>> struct netconfig_list *nc_configs; /* pointer to the current >>> netconfig entry */ >>> >>> }; >>> >>> +struct nc_thread_vars { >>> >>> + int nc_error; >>> >>> + FILE *nc_file; >>> >>> + struct netconfig_info nc_ni; >>> >>> +}; >>> >>> + >>> >>> #define NC_VALID 0xfeed >>> >>> #define NC_STORAGE 0xf00d >>> >>> #define NC_INVALID 0 >>> >>> >>> >>> -static int *__nc_error(void); >>> >>> static int parse_ncp(char *, struct netconfig *); >>> >>> static struct netconfig *dup_ncp(struct netconfig *); >>> >>> >>> >>> -static FILE *nc_file; /* for netconfig db */ >>> >>> -static mutex_t nc_file_lock = MUTEX_INITIALIZER; >>> >>> - >>> >>> -static struct netconfig_info ni = { 0, 0, NULL, NULL}; >>> >>> -static mutex_t ni_lock = MUTEX_INITIALIZER; >>> >>> - >>> >>> static thread_key_t nc_key; >>> >>> static once_t nc_once = ONCE_INITIALIZER; >>> >>> static int nc_key_error; >>> >>> @@ -148,34 +147,37 @@ nc_key_init(void) >>> >>> #define MAXNETCONFIGLINE 1000 >>> >>> -static int * >>> >>> -__nc_error() >>> >>> +static struct nc_thread_vars * >>> >>> +__nc_thread() >>> >>> { >>> >>> - static int nc_error = 0; >>> >>> - int *nc_addr; >>> >>> + static struct nc_thread_vars nc_vars = {0, NULL, {0, 0, >>> NULL, NULL}}; >>> >>> + struct nc_thread_vars *nc_addr; >>> >>> /* >>> >>> - * Use the static `nc_error' if we are the main thread >>> >>> + * Use the static `nc_vars' if we are the main thread >>> >>> * (including non-threaded programs), or if an allocation >>> >>> * fails. >>> >>> */ >>> >>> if (thr_main()) >>> >>> - return (&nc_error); >>> >>> + return (&nc_vars); >>> >>> if (thr_once(&nc_once, nc_key_init) != 0 || nc_key_error >>> != 0) >>> >>> - return (&nc_error); >>> >>> - if ((nc_addr = (int *)thr_getspecific(nc_key)) == NULL) { >>> >>> - nc_addr = (int *)malloc(sizeof (int)); >>> >>> + return (&nc_vars); >>> >>> + if ((nc_addr = (struct nc_thread_vars >>> *)thr_getspecific(nc_key)) >>> >>> + == NULL) { >>> >>> + nc_addr = (struct nc_thread_vars *) >>> >>> + calloc(1, sizeof(struct >>> nc_thread_vars)); >>> >>> if (thr_setspecific(nc_key, (void *) >>> nc_addr) != 0) { >>> >>> if (nc_addr) >>> >>> >>> free(nc_addr); >>> >>> - return (&nc_error); >>> >>> + return (&nc_vars); >>> >>> } >>> >>> - *nc_addr = 0; >>> >>> } >>> >>> return (nc_addr); >>> >>> } >>> >>> -#define nc_error (*(__nc_error())) >>> >>> +#define nc_error (*(&(__nc_thread()->nc_error))) >>> >>> +#define nc_file (*(&(__nc_thread()->nc_file))) >>> >>> +#define ni (*(&(__nc_thread()->nc_ni))) >>> >>> /* >>> >>> * A call to setnetconfig() establishes a /etc/netconfig "session". A >>> session >>> >>> * "handle" is returned on a successful call. At the start of a >>> session (after >>> >>> @@ -209,23 +211,14 @@ setnetconfig() >>> >>> * For multiple calls, i.e. nc_file is not NULL, we just return the >>> >>> * handle without reopening the netconfig db. >>> >>> */ >>> >>> - mutex_lock(&ni_lock); >>> >>> ni.ref++; >>> >>> - mutex_unlock(&ni_lock); >>> >>> - >>> >>> - mutex_lock(&nc_file_lock); >>> >>> if ((nc_file != NULL) || (nc_file = fopen(NETCONFIG, "r")) != NULL) >>> { >>> >>> nc_vars->valid = NC_VALID; >>> >>> nc_vars->flag = 0; >>> >>> nc_vars->nc_configs = ni.head; >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> return ((void *)nc_vars); >>> >>> } >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> - >>> >>> - mutex_lock(&ni_lock); >>> >>> ni.ref--; >>> >>> - mutex_unlock(&ni_lock); >>> >>> nc_error = NC_NONETCONFIG; >>> >>> free(nc_vars); >>> >>> @@ -254,13 +247,10 @@ void *handlep; >>> >>> /* >>> >>> * Verify that handle is valid >>> >>> */ >>> >>> - mutex_lock(&nc_file_lock); >>> >>> if (ncp == NULL || nc_file == NULL) { >>> >>> nc_error = NC_NOTINIT; >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> return (NULL); >>> >>> } >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> switch (ncp->valid) { >>> >>> case NC_VALID: >>> >>> @@ -274,9 +264,7 @@ void *handlep; >>> >>> */ >>> >>> if (ncp->flag == 0) { /* first time */ >>> >>> ncp->flag = 1; >>> >>> - mutex_lock(&ni_lock); >>> >>> ncp->nc_configs = ni.head; >>> >>> - mutex_unlock(&ni_lock); >>> >>> if (ncp->nc_configs != NULL) /* entry already exist >>> */ >>> >>> return(ncp->nc_configs->ncp); >>> >>> } >>> >>> @@ -289,12 +277,9 @@ void *handlep; >>> >>> * If we cannot find the entry in the list and is end of >>> file, >>> >>> * we give up. >>> >>> */ >>> >>> - mutex_lock(&ni_lock); >>> >>> if (ni.eof == 1) { >>> >>> - mutex_unlock(&ni_lock); >>> >>> return(NULL); >>> >>> } >>> >>> - mutex_unlock(&ni_lock); >>> >>> break; >>> >>> default: >>> >>> @@ -316,18 +301,13 @@ void *handlep; >>> >>> /* >>> >>> * Read a line from netconfig file. >>> >>> */ >>> >>> - mutex_lock(&nc_file_lock); >>> >>> do { >>> >>> if (fgets(stringp, MAXNETCONFIGLINE, nc_file) == NULL) { >>> >>> free(stringp); >>> >>> - mutex_lock(&ni_lock); >>> >>> ni.eof = 1; >>> >>> - mutex_unlock(&ni_lock); >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> return (NULL); >>> >>> } >>> >>> } while (*stringp == '#'); >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> list = (struct netconfig_list *) malloc(sizeof (struct >>> netconfig_list)); >>> >>> if (list == NULL) { >>> >>> @@ -357,7 +337,6 @@ void *handlep; >>> >>> * Reposition the current pointer of the handle to the >>> last entry >>> >>> * in the list. >>> >>> */ >>> >>> - mutex_lock(&ni_lock); >>> >>> if (ni.head == NULL) { /* first entry */ >>> >>> ni.head = ni.tail = list; >>> >>> } >>> >>> @@ -367,7 +346,6 @@ void *handlep; >>> >>> } >>> >>> ncp->nc_configs = ni.tail; >>> >>> result = ni.tail->ncp; >>> >>> - mutex_unlock(&ni_lock); >>> >>> return(result); >>> >>> } >>> >>> } >>> >>> @@ -402,9 +380,7 @@ void *handlep; >>> >>> nc_handlep->valid = NC_INVALID; >>> >>> nc_handlep->flag = 0; >>> >>> nc_handlep->nc_configs = NULL; >>> >>> - mutex_lock(&ni_lock); >>> >>> if (--ni.ref > 0) { >>> >>> - mutex_unlock(&ni_lock); >>> >>> free(nc_handlep); >>> >>> return(0); >>> >>> } >>> >>> @@ -417,7 +393,6 @@ void *handlep; >>> >>> ni.eof = ni.ref = 0; >>> >>> ni.head = NULL; >>> >>> ni.tail = NULL; >>> >>> - mutex_unlock(&ni_lock); >>> >>> while (q != NULL) { >>> >>> p = q->next; >>> >>> @@ -429,10 +404,8 @@ void *handlep; >>> >>> } >>> >>> free(nc_handlep); >>> >>> - mutex_lock(&nc_file_lock); >>> >>> fclose(nc_file); >>> >>> nc_file = NULL; >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> return (0); >>> >>> } >>> >>> @@ -481,21 +454,16 @@ getnetconfigent(netid) >>> >>> * If all the netconfig db has been read and placed into the list >>> and >>> >>> * there is no match for the netid, return NULL. >>> >>> */ >>> >>> - mutex_lock(&ni_lock); >>> >>> if (ni.head != NULL) { >>> >>> for (list = ni.head; list; list = list->next) { >>> >>> if (strcmp(list->ncp->nc_netid, netid) == 0) { >>> >>> - mutex_unlock(&ni_lock); >>> >>> return(dup_ncp(list->ncp)); >>> >>> } >>> >>> } >>> >>> if (ni.eof == 1) { /* that's all the entries */ >>> >>> - mutex_unlock(&ni_lock); >>> >>> return(NULL); >>> >>> } >>> >>> } >>> >>> - mutex_unlock(&ni_lock); >>> >>> - >>> >>> if ((file = fopen(NETCONFIG, "r")) == NULL) { >>> >>> nc_error = NC_NONETCONFIG; >>> >>> -- >>> >>> 1.7.9.5 >>> >>> >>> >>> _______________________________________________ >>> freebsd-threads@freebsd.org mailing list >>> http://lists.freebsd.org/mailman/listinfo/freebsd-threads >>> To unsubscribe, send any mail to "freebsd-threads-unsubscribe@freebsd.org" >> >> -- >> - Alfred Perlstein >> .- VMOA #5191, 03 vmax, 92 gs500, 85 ch250, 07 zx10 >> .- FreeBSD committer >> _______________________________________________ >> freebsd-threads@freebsd.org mailing list >> http://lists.freebsd.org/mailman/listinfo/freebsd-threads >> To unsubscribe, send any mail to "freebsd-threads-unsubscribe@freebsd.org" > From owner-freebsd-threads@FreeBSD.ORG Fri Aug 24 22:18:23 2012 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id E775B106564A; Fri, 24 Aug 2012 22:18:23 +0000 (UTC) (envelope-from steven.haber@isilon.com) Received: from mexforward.lss.emc.com (hop-nat-141.emc.com [168.159.213.141]) by mx1.freebsd.org (Postfix) with ESMTP id 27E678FC12; Fri, 24 Aug 2012 22:18:23 +0000 (UTC) Received: from hop04-l1d11-si03.isus.emc.com (HOP04-L1D11-SI03.isus.emc.com [10.254.111.23]) by mexforward.lss.emc.com (Switch-3.4.3/Switch-3.4.3) with ESMTP id q7OMIMrM008875 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 24 Aug 2012 18:18:22 -0400 Received: from mailhub.lss.emc.com (mailhub.lss.emc.com [10.254.222.130]) by hop04-l1d11-si03.isus.emc.com (RSA Interceptor); Fri, 24 Aug 2012 18:18:05 -0400 Received: from seacasht02.desktop.isilon.com (seacasht02.isilon.com [137.69.159.81]) by mailhub.lss.emc.com (Switch-3.4.3/Switch-3.4.3) with ESMTP id q7OMHxvs028631; Fri, 24 Aug 2012 18:18:04 -0400 Received: from seaxch01.isilon.com (137.69.158.60) by SEACASHT02.desktop.isilon.com (137.69.159.81) with Microsoft SMTP Server id 14.2.298.4; Fri, 24 Aug 2012 15:15:24 -0700 Received: from [10.7.181.140] ([10.7.181.140]) by seaxch01.isilon.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 24 Aug 2012 15:14:45 -0700 Message-ID: <5037FC88.3080608@isilon.com> Date: Fri, 24 Aug 2012 15:13:28 -0700 From: Steven Haber User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120714 Thunderbird/14.0 MIME-Version: 1.0 To: Alfred Perlstein References: <56CE86D6660FF84498426FA7A33170B40413DF8E@seaxch01.desktop.isilon.com> <20120824200714.GM68801@elvis.mu.org> <20120824201745.GO68801@elvis.mu.org> In-Reply-To: <20120824201745.GO68801@elvis.mu.org> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-OriginalArrivalTime: 24 Aug 2012 22:14:45.0331 (UTC) FILETIME=[DE0C3A30:01CD8245] X-EMM-MHVC: 1 Cc: snnn119@gmail.com, freebsd-threads@freebsd.org Subject: Re: netconfig is not threadsafe X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Aug 2012 22:18:24 -0000 Yeah, I definitely think it should support threading. It looks like we only use netconfig a few places: RPC, and some sbin services like mountd, yp, and keyserv. So we could just serialize correctly in RPC/mountd/etc., but I think it's better to protect the whole thing. Linux (Ubuntu) has a FreeBSD man page for netconfig with no mention of _r routines; the APIs look pretty much like they do on FreeBSD. I can't seem to find the source in my libc, however. For netgrent (similar to netconfig) there is getnetgrent_r(), a GNU-specific call, which just stores retrieved strings in a buffer you pass in, but near as I can tell the calls aren't actually thread safe: http://sourceware.org/git/?p=glibc.git;a=blob;f=inet/getnetgrent_r.c;h=8e69ec7b1135dc886a1cf0730001fd709a701b87;hb=HEAD All the data is just stored in a static, and calls to set/get/endnetgrent are serialized by a lock, but you could call them out of order with multiple threads. In Solaris 10, you get a handle back from setnetconfig() that is unique to your session. This implementation sounds fully threadsafe. Haven't tried it. On OSX 10.7.2 the calls look the same as FreeBSD (no handle) and it does not mention whether or not it is threadsafe, though in BUGS it mentions: "The function getnetgrent() returns pointers to dynamically allocated data areas that are freed when the function endnetgrent() is called." I haven't tried threading it on OSX, either. I'm pasting a better version of the patch since Outlook destroyed my last one. Steven From f3639afbe7993f3c699f23f4d77749ea01e8e8c8 Mon Sep 17 00:00:00 2001 From: Steven Haber Date: Thu, 23 Aug 2012 17:45:51 -0700 Subject: [PATCH] Fix for bug 95757. Make netconfig threadsafe so we can make lots of RPC clients concurrently. The original locking had no effect; there is no point in protecting the static members since you should never have two threads in the same get/set/endnetconfig. The point of these functions is to be called in sequence as a transaction. You can't have multiple threads calling them if the state is stored as a process global. This fix moves the static vars to thread- specific storage so that each thread can have its own transaction, and removes the existing locking. --- src/lib/libc/rpc/getnetconfig.c | 74 +++++++++++---------------------------- 1 file changed, 21 insertions(+), 53 deletions(-) diff --git a/src/lib/libc/rpc/getnetconfig.c b/src/lib/libc/rpc/getnetconfig.c index 0f7168f..2957d14 100644 --- a/src/lib/libc/rpc/getnetconfig.c +++ b/src/lib/libc/rpc/getnetconfig.c @@ -119,22 +119,21 @@ struct netconfig_vars { struct netconfig_list *nc_configs; /* pointer to the current netconfig entry */ }; +struct nc_thread_vars { + int nc_error; + FILE *nc_file; + struct netconfig_info nc_ni; +}; + #define NC_VALID 0xfeed #define NC_STORAGE 0xf00d #define NC_INVALID 0 -static int *__nc_error(void); static int parse_ncp(char *, struct netconfig *); static struct netconfig *dup_ncp(struct netconfig *); -static FILE *nc_file; /* for netconfig db */ -static mutex_t nc_file_lock = MUTEX_INITIALIZER; - -static struct netconfig_info ni = { 0, 0, NULL, NULL}; -static mutex_t ni_lock = MUTEX_INITIALIZER; - static thread_key_t nc_key; static once_t nc_once = ONCE_INITIALIZER; static int nc_key_error; @@ -148,34 +147,37 @@ nc_key_init(void) #define MAXNETCONFIGLINE 1000 -static int * -__nc_error() +static struct nc_thread_vars * +__nc_thread() { - static int nc_error = 0; - int *nc_addr; + static struct nc_thread_vars nc_vars = {0, NULL, {0, 0, NULL, NULL}}; + struct nc_thread_vars *nc_addr; /* - * Use the static `nc_error' if we are the main thread + * Use the static `nc_vars' if we are the main thread * (including non-threaded programs), or if an allocation * fails. */ if (thr_main()) - return (&nc_error); + return (&nc_vars); if (thr_once(&nc_once, nc_key_init) != 0 || nc_key_error != 0) - return (&nc_error); - if ((nc_addr = (int *)thr_getspecific(nc_key)) == NULL) { - nc_addr = (int *)malloc(sizeof (int)); + return (&nc_vars); + if ((nc_addr = (struct nc_thread_vars *)thr_getspecific(nc_key)) + == NULL) { + nc_addr = (struct nc_thread_vars *) + calloc(1, sizeof(struct nc_thread_vars)); if (thr_setspecific(nc_key, (void *) nc_addr) != 0) { if (nc_addr) free(nc_addr); - return (&nc_error); + return (&nc_vars); } - *nc_addr = 0; } return (nc_addr); } -#define nc_error (*(__nc_error())) +#define nc_error (*(&(__nc_thread()->nc_error))) +#define nc_file (*(&(__nc_thread()->nc_file))) +#define ni (*(&(__nc_thread()->nc_ni))) /* * A call to setnetconfig() establishes a /etc/netconfig "session". A session * "handle" is returned on a successful call. At the start of a session (after @@ -209,23 +211,14 @@ setnetconfig() * For multiple calls, i.e. nc_file is not NULL, we just return the * handle without reopening the netconfig db. */ - mutex_lock(&ni_lock); ni.ref++; - mutex_unlock(&ni_lock); - - mutex_lock(&nc_file_lock); if ((nc_file != NULL) || (nc_file = fopen(NETCONFIG, "r")) != NULL) { nc_vars->valid = NC_VALID; nc_vars->flag = 0; nc_vars->nc_configs = ni.head; - mutex_unlock(&nc_file_lock); return ((void *)nc_vars); } - mutex_unlock(&nc_file_lock); - - mutex_lock(&ni_lock); ni.ref--; - mutex_unlock(&ni_lock); nc_error = NC_NONETCONFIG; free(nc_vars); @@ -254,13 +247,10 @@ void *handlep; /* * Verify that handle is valid */ - mutex_lock(&nc_file_lock); if (ncp == NULL || nc_file == NULL) { nc_error = NC_NOTINIT; - mutex_unlock(&nc_file_lock); return (NULL); } - mutex_unlock(&nc_file_lock); switch (ncp->valid) { case NC_VALID: @@ -274,9 +264,7 @@ void *handlep; */ if (ncp->flag == 0) { /* first time */ ncp->flag = 1; - mutex_lock(&ni_lock); ncp->nc_configs = ni.head; - mutex_unlock(&ni_lock); if (ncp->nc_configs != NULL) /* entry already exist */ return(ncp->nc_configs->ncp); } @@ -289,12 +277,9 @@ void *handlep; * If we cannot find the entry in the list and is end of file, * we give up. */ - mutex_lock(&ni_lock); if (ni.eof == 1) { - mutex_unlock(&ni_lock); return(NULL); } - mutex_unlock(&ni_lock); break; default: @@ -316,18 +301,13 @@ void *handlep; /* * Read a line from netconfig file. */ - mutex_lock(&nc_file_lock); do { if (fgets(stringp, MAXNETCONFIGLINE, nc_file) == NULL) { free(stringp); - mutex_lock(&ni_lock); ni.eof = 1; - mutex_unlock(&ni_lock); - mutex_unlock(&nc_file_lock); return (NULL); } } while (*stringp == '#'); - mutex_unlock(&nc_file_lock); list = (struct netconfig_list *) malloc(sizeof (struct netconfig_list)); if (list == NULL) { @@ -357,7 +337,6 @@ void *handlep; * Reposition the current pointer of the handle to the last entry * in the list. */ - mutex_lock(&ni_lock); if (ni.head == NULL) { /* first entry */ ni.head = ni.tail = list; } @@ -367,7 +346,6 @@ void *handlep; } ncp->nc_configs = ni.tail; result = ni.tail->ncp; - mutex_unlock(&ni_lock); return(result); } } @@ -402,9 +380,7 @@ void *handlep; nc_handlep->valid = NC_INVALID; nc_handlep->flag = 0; nc_handlep->nc_configs = NULL; - mutex_lock(&ni_lock); if (--ni.ref > 0) { - mutex_unlock(&ni_lock); free(nc_handlep); return(0); } @@ -417,7 +393,6 @@ void *handlep; ni.eof = ni.ref = 0; ni.head = NULL; ni.tail = NULL; - mutex_unlock(&ni_lock); while (q != NULL) { p = q->next; @@ -429,10 +404,8 @@ void *handlep; } free(nc_handlep); - mutex_lock(&nc_file_lock); fclose(nc_file); nc_file = NULL; - mutex_unlock(&nc_file_lock); return (0); } @@ -481,21 +454,16 @@ getnetconfigent(netid) * If all the netconfig db has been read and placed into the list and * there is no match for the netid, return NULL. */ - mutex_lock(&ni_lock); if (ni.head != NULL) { for (list = ni.head; list; list = list->next) { if (strcmp(list->ncp->nc_netid, netid) == 0) { - mutex_unlock(&ni_lock); return(dup_ncp(list->ncp)); } } if (ni.eof == 1) { /* that's all the entries */ - mutex_unlock(&ni_lock); return(NULL); } } - mutex_unlock(&ni_lock); - if ((file = fopen(NETCONFIG, "r")) == NULL) { nc_error = NC_NONETCONFIG; -- 1.7.9.5 On 08/24/2012 01:17 PM, Alfred Perlstein wrote: > I did a bit of research and this manual page: > http://www.unix.com/man-page/All/3n/setnetconfig/ > > seems to indicate that this is the best that is "expected" > for thread safety: > > MULTITHREAD USAGE > Thread Safe: Yes > Cancel Safe: Yes > Fork Safe: No > Async-cancel Safe: No > Async-signal Safe: No > > These functions can be called safely in a multithreaded environment. > They may be cancellation points in that they call functions that are > cancel points. > > In a multithreaded environment, these functions are not safe to be > called by a child process after and before These functions should not > be called by a multithreaded application that supports asynchronous > cancellation or asynchronous signals. > > So this is awesome. Thank you. > > -Alfred > > * Alfred Perlstein [120824 13:14] wrote: >> I suppose it can be fixed like this and the idea is pretty sound, >> however I was wondering if you can you look at how Linux/Solaris/OSX >> handle this? >> >> I would think there would be some form of "_r" routines that they >> would use instead. Or maybe thier libc avoids calling these routines >> somehow? >> >> That said this is good work and should go in barring other >> implementations that may make more sense. >> >> -Alfred >> >> * Steven Haber [120824 13:00] wrote: >>> netconfig (libc routines for reading the netconfig file) is currently >>> not threadsafe. It is a transactional API set, that is, you call >>> setnetconfig() to open the file, read from the file with getnetconfig(), >>> and close/cleanup with endnetconfig(). The problem is that the order >>> that these routines are called in is important, but the state of the >>> current transaction is stored in a per-process static, so only a single >>> thread can do a transaction at one time. A patch from five years ago >>> attempted to fix this: >>> >>> >>> >>> http://lists.freebsd.org/mailman/confirm/freebsd-threads/292f06b38808587 >>> 41c9034fb4173a14740c63a9e >>> >>> >>> >>> This patch does not solve the problem. It adds protection for the >>> statics so that concurrent calls into set/get/endnetgrent() don't trash >>> the statics. However, there is no use case where you would want two >>> threads accessing any of these statics concurrently. To solve this >>> problem, I have stored the FILE and netconfig_info statics on a >>> per-thread basis using the pthreads key setup that was already in place >>> for the error code. Now threads have their own set of stored state data, >>> so no statics are necessary. I removed the original locking that had no >>> effect. >>> >>> >>> >>> My repro is to spawn 32 threads that all create an RPC client >>> simultaneously using libc's clnt_create(), which calls into netconfig. >>> After patching, I no longer see the issue. Here is the stack when a >>> thread collision occurs (same as in the original bug from 2007): >>> >>> >>> >>> #0 fclose (fp=0x0) at >>> /root/Source/onefs/git/src/lib/libc/stdio/fclose.c:52 >>> >>> #1 0x0000000802c83a79 in endnetconfig (handlep=0x80b1030a0) at >>> /root/Source/onefs/git/src/lib/libc/rpc/getnetconfig.c:433 >>> >>> #2 0x0000000802c811d0 in __rpc_endconf (vhandle=0x80b103090) at >>> /root/Source/onefs/git/src/lib/libc/rpc/rpc_generic.c:445 >>> >>> #3 0x0000000802c7331a in clnt_create_timed (hostname=0x804d7cd9b >>> "tgroup %s", prog=134420864, vers=1, netclass=Variable "netclass" is not >>> available. >>> >>> ) at /root/Source/onefs/git/src/lib/libc/rpc/clnt_generic.c:271 >>> >>> >>> >>> This issue is present in FreeBSD head, though I'm reproducing on 7.1. >>> >>> >>> >>> Does the patch make sense? Should I open a bug on this somehow? I'm >>> relatively new to contributing to FreeBSD. >>> >>> >>> >>> Thanks, >>> >>> Steven >>> >>> >>> >>> >>> >>> >From f3639afbe7993f3c699f23f4d77749ea01e8e8c8 Mon Sep 17 00:00:00 2001 >>> >>> From: Steven Haber >>> >>> Date: Thu, 23 Aug 2012 17:45:51 -0700 >>> >>> Subject: [PATCH] Fix for bug 95757. Make netconfig threadsafe so we can >>> make >>> >>> lots of RPC clients concurrently. The original locking had >>> >>> no effect; there is no point in protecting the static >>> >>> members since you should never have two threads in the same >>> >>> get/set/endnetconfig. The point of these functions is to be >>> >>> called in sequence as a transaction. You can't have >>> >>> multiple threads calling them if the state is stored as a >>> >>> process global. This fix moves the static vars to thread- >>> >>> specific storage so that each thread can have its own >>> >>> transaction, and removes the exiting locking. >>> >>> --- >>> >>> src/lib/libc/rpc/getnetconfig.c | 74 >>> +++++++++++---------------------------- >>> >>> 1 file changed, 21 insertions(+), 53 deletions(-) >>> >>> >>> >>> diff --git a/src/lib/libc/rpc/getnetconfig.c >>> b/src/lib/libc/rpc/getnetconfig.c >>> >>> index 0f7168f..2957d14 100644 >>> >>> --- a/src/lib/libc/rpc/getnetconfig.c >>> >>> +++ b/src/lib/libc/rpc/getnetconfig.c >>> >>> @@ -119,22 +119,21 @@ struct netconfig_vars { >>> >>> struct netconfig_list *nc_configs; /* pointer to the current >>> netconfig entry */ >>> >>> }; >>> >>> +struct nc_thread_vars { >>> >>> + int nc_error; >>> >>> + FILE *nc_file; >>> >>> + struct netconfig_info nc_ni; >>> >>> +}; >>> >>> + >>> >>> #define NC_VALID 0xfeed >>> >>> #define NC_STORAGE 0xf00d >>> >>> #define NC_INVALID 0 >>> >>> >>> >>> -static int *__nc_error(void); >>> >>> static int parse_ncp(char *, struct netconfig *); >>> >>> static struct netconfig *dup_ncp(struct netconfig *); >>> >>> >>> >>> -static FILE *nc_file; /* for netconfig db */ >>> >>> -static mutex_t nc_file_lock = MUTEX_INITIALIZER; >>> >>> - >>> >>> -static struct netconfig_info ni = { 0, 0, NULL, NULL}; >>> >>> -static mutex_t ni_lock = MUTEX_INITIALIZER; >>> >>> - >>> >>> static thread_key_t nc_key; >>> >>> static once_t nc_once = ONCE_INITIALIZER; >>> >>> static int nc_key_error; >>> >>> @@ -148,34 +147,37 @@ nc_key_init(void) >>> >>> #define MAXNETCONFIGLINE 1000 >>> >>> -static int * >>> >>> -__nc_error() >>> >>> +static struct nc_thread_vars * >>> >>> +__nc_thread() >>> >>> { >>> >>> - static int nc_error = 0; >>> >>> - int *nc_addr; >>> >>> + static struct nc_thread_vars nc_vars = {0, NULL, {0, 0, >>> NULL, NULL}}; >>> >>> + struct nc_thread_vars *nc_addr; >>> >>> /* >>> >>> - * Use the static `nc_error' if we are the main thread >>> >>> + * Use the static `nc_vars' if we are the main thread >>> >>> * (including non-threaded programs), or if an allocation >>> >>> * fails. >>> >>> */ >>> >>> if (thr_main()) >>> >>> - return (&nc_error); >>> >>> + return (&nc_vars); >>> >>> if (thr_once(&nc_once, nc_key_init) != 0 || nc_key_error >>> != 0) >>> >>> - return (&nc_error); >>> >>> - if ((nc_addr = (int *)thr_getspecific(nc_key)) == NULL) { >>> >>> - nc_addr = (int *)malloc(sizeof (int)); >>> >>> + return (&nc_vars); >>> >>> + if ((nc_addr = (struct nc_thread_vars >>> *)thr_getspecific(nc_key)) >>> >>> + == NULL) { >>> >>> + nc_addr = (struct nc_thread_vars *) >>> >>> + calloc(1, sizeof(struct >>> nc_thread_vars)); >>> >>> if (thr_setspecific(nc_key, (void *) >>> nc_addr) != 0) { >>> >>> if (nc_addr) >>> >>> >>> free(nc_addr); >>> >>> - return (&nc_error); >>> >>> + return (&nc_vars); >>> >>> } >>> >>> - *nc_addr = 0; >>> >>> } >>> >>> return (nc_addr); >>> >>> } >>> >>> -#define nc_error (*(__nc_error())) >>> >>> +#define nc_error (*(&(__nc_thread()->nc_error))) >>> >>> +#define nc_file (*(&(__nc_thread()->nc_file))) >>> >>> +#define ni (*(&(__nc_thread()->nc_ni))) >>> >>> /* >>> >>> * A call to setnetconfig() establishes a /etc/netconfig "session". A >>> session >>> >>> * "handle" is returned on a successful call. At the start of a >>> session (after >>> >>> @@ -209,23 +211,14 @@ setnetconfig() >>> >>> * For multiple calls, i.e. nc_file is not NULL, we just return the >>> >>> * handle without reopening the netconfig db. >>> >>> */ >>> >>> - mutex_lock(&ni_lock); >>> >>> ni.ref++; >>> >>> - mutex_unlock(&ni_lock); >>> >>> - >>> >>> - mutex_lock(&nc_file_lock); >>> >>> if ((nc_file != NULL) || (nc_file = fopen(NETCONFIG, "r")) != NULL) >>> { >>> >>> nc_vars->valid = NC_VALID; >>> >>> nc_vars->flag = 0; >>> >>> nc_vars->nc_configs = ni.head; >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> return ((void *)nc_vars); >>> >>> } >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> - >>> >>> - mutex_lock(&ni_lock); >>> >>> ni.ref--; >>> >>> - mutex_unlock(&ni_lock); >>> >>> nc_error = NC_NONETCONFIG; >>> >>> free(nc_vars); >>> >>> @@ -254,13 +247,10 @@ void *handlep; >>> >>> /* >>> >>> * Verify that handle is valid >>> >>> */ >>> >>> - mutex_lock(&nc_file_lock); >>> >>> if (ncp == NULL || nc_file == NULL) { >>> >>> nc_error = NC_NOTINIT; >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> return (NULL); >>> >>> } >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> switch (ncp->valid) { >>> >>> case NC_VALID: >>> >>> @@ -274,9 +264,7 @@ void *handlep; >>> >>> */ >>> >>> if (ncp->flag == 0) { /* first time */ >>> >>> ncp->flag = 1; >>> >>> - mutex_lock(&ni_lock); >>> >>> ncp->nc_configs = ni.head; >>> >>> - mutex_unlock(&ni_lock); >>> >>> if (ncp->nc_configs != NULL) /* entry already exist >>> */ >>> >>> return(ncp->nc_configs->ncp); >>> >>> } >>> >>> @@ -289,12 +277,9 @@ void *handlep; >>> >>> * If we cannot find the entry in the list and is end of >>> file, >>> >>> * we give up. >>> >>> */ >>> >>> - mutex_lock(&ni_lock); >>> >>> if (ni.eof == 1) { >>> >>> - mutex_unlock(&ni_lock); >>> >>> return(NULL); >>> >>> } >>> >>> - mutex_unlock(&ni_lock); >>> >>> break; >>> >>> default: >>> >>> @@ -316,18 +301,13 @@ void *handlep; >>> >>> /* >>> >>> * Read a line from netconfig file. >>> >>> */ >>> >>> - mutex_lock(&nc_file_lock); >>> >>> do { >>> >>> if (fgets(stringp, MAXNETCONFIGLINE, nc_file) == NULL) { >>> >>> free(stringp); >>> >>> - mutex_lock(&ni_lock); >>> >>> ni.eof = 1; >>> >>> - mutex_unlock(&ni_lock); >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> return (NULL); >>> >>> } >>> >>> } while (*stringp == '#'); >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> list = (struct netconfig_list *) malloc(sizeof (struct >>> netconfig_list)); >>> >>> if (list == NULL) { >>> >>> @@ -357,7 +337,6 @@ void *handlep; >>> >>> * Reposition the current pointer of the handle to the >>> last entry >>> >>> * in the list. >>> >>> */ >>> >>> - mutex_lock(&ni_lock); >>> >>> if (ni.head == NULL) { /* first entry */ >>> >>> ni.head = ni.tail = list; >>> >>> } >>> >>> @@ -367,7 +346,6 @@ void *handlep; >>> >>> } >>> >>> ncp->nc_configs = ni.tail; >>> >>> result = ni.tail->ncp; >>> >>> - mutex_unlock(&ni_lock); >>> >>> return(result); >>> >>> } >>> >>> } >>> >>> @@ -402,9 +380,7 @@ void *handlep; >>> >>> nc_handlep->valid = NC_INVALID; >>> >>> nc_handlep->flag = 0; >>> >>> nc_handlep->nc_configs = NULL; >>> >>> - mutex_lock(&ni_lock); >>> >>> if (--ni.ref > 0) { >>> >>> - mutex_unlock(&ni_lock); >>> >>> free(nc_handlep); >>> >>> return(0); >>> >>> } >>> >>> @@ -417,7 +393,6 @@ void *handlep; >>> >>> ni.eof = ni.ref = 0; >>> >>> ni.head = NULL; >>> >>> ni.tail = NULL; >>> >>> - mutex_unlock(&ni_lock); >>> >>> while (q != NULL) { >>> >>> p = q->next; >>> >>> @@ -429,10 +404,8 @@ void *handlep; >>> >>> } >>> >>> free(nc_handlep); >>> >>> - mutex_lock(&nc_file_lock); >>> >>> fclose(nc_file); >>> >>> nc_file = NULL; >>> >>> - mutex_unlock(&nc_file_lock); >>> >>> return (0); >>> >>> } >>> >>> @@ -481,21 +454,16 @@ getnetconfigent(netid) >>> >>> * If all the netconfig db has been read and placed into the list >>> and >>> >>> * there is no match for the netid, return NULL. >>> >>> */ >>> >>> - mutex_lock(&ni_lock); >>> >>> if (ni.head != NULL) { >>> >>> for (list = ni.head; list; list = list->next) { >>> >>> if (strcmp(list->ncp->nc_netid, netid) == 0) { >>> >>> - mutex_unlock(&ni_lock); >>> >>> return(dup_ncp(list->ncp)); >>> >>> } >>> >>> } >>> >>> if (ni.eof == 1) { /* that's all the entries */ >>> >>> - mutex_unlock(&ni_lock); >>> >>> return(NULL); >>> >>> } >>> >>> } >>> >>> - mutex_unlock(&ni_lock); >>> >>> - >>> >>> if ((file = fopen(NETCONFIG, "r")) == NULL) { >>> >>> nc_error = NC_NONETCONFIG; >>> >>> -- >>> >>> 1.7.9.5 >>> >>> >>> >>> _______________________________________________ >>> freebsd-threads@freebsd.org mailing list >>> http://lists.freebsd.org/mailman/listinfo/freebsd-threads >>> To unsubscribe, send any mail to "freebsd-threads-unsubscribe@freebsd.org" >> >> -- >> - Alfred Perlstein >> .- VMOA #5191, 03 vmax, 92 gs500, 85 ch250, 07 zx10 >> .- FreeBSD committer >> _______________________________________________ >> freebsd-threads@freebsd.org mailing list >> http://lists.freebsd.org/mailman/listinfo/freebsd-threads >> To unsubscribe, send any mail to "freebsd-threads-unsubscribe@freebsd.org" >