From owner-freebsd-i18n@FreeBSD.ORG Wed Feb 27 21:40:57 2013 Return-Path: Delivered-To: freebsd-i18n@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id EBA1F169 for ; Wed, 27 Feb 2013 21:40:57 +0000 (UTC) (envelope-from jmg@h2.funkthat.com) Received: from h2.funkthat.com (gate2.funkthat.com [208.87.223.18]) by mx1.freebsd.org (Postfix) with ESMTP id B8EDB1DC for ; Wed, 27 Feb 2013 21:40:57 +0000 (UTC) Received: from h2.funkthat.com (localhost [127.0.0.1]) by h2.funkthat.com (8.14.3/8.14.3) with ESMTP id r1RLevuI008831 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Wed, 27 Feb 2013 13:40:57 -0800 (PST) (envelope-from jmg@h2.funkthat.com) Received: (from jmg@localhost) by h2.funkthat.com (8.14.3/8.14.3/Submit) id r1RLevqX008830 for freebsd-i18n@FreeBSD.org; Wed, 27 Feb 2013 13:40:57 -0800 (PST) (envelope-from jmg) Date: Wed, 27 Feb 2013 13:40:57 -0800 From: John-Mark Gurney To: freebsd-i18n@FreeBSD.org Subject: Re: patch for in tree iconv Message-ID: <20130227214057.GD55866@funkthat.com> References: <20130227213200.GB55866@funkthat.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="4f28nU6agdXSinmL" Content-Disposition: inline In-Reply-To: <20130227213200.GB55866@funkthat.com> User-Agent: Mutt/1.4.2.3i X-Operating-System: FreeBSD 7.2-RELEASE i386 X-PGP-Fingerprint: 54BA 873B 6515 3F10 9E88 9322 9CB1 8F74 6D3F A396 X-Files: The truth is out there X-URL: http://resnet.uoregon.edu/~gurney_j/ X-Resume: http://resnet.uoregon.edu/~gurney_j/resume.html X-to-the-FBI-CIA-and-NSA: HI! HOW YA DOIN? can i haz chizburger? X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.2.2 (h2.funkthat.com [127.0.0.1]); Wed, 27 Feb 2013 13:40:57 -0800 (PST) X-BeenThere: freebsd-i18n@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: FreeBSD Internationalization Effort List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Feb 2013 21:40:58 -0000 --4f28nU6agdXSinmL Content-Type: text/plain; charset=us-ascii Content-Disposition: inline John-Mark Gurney wrote this message on Wed, Feb 27, 2013 at 13:32 -0800: > I've worked at fixing a few of iconv's locking issues, and I've attached > this patch. This should fix most/all of the locking related issues > relating to the global lock... I should of included the patch... Oops... In case the patch gets stripped: http://people.FreeBSD.org/~jmg/iconv.patch -- John-Mark Gurney Voice: +1 415 225 5579 "All that I will do, has been done, All that I have, has not." --4f28nU6agdXSinmL Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="iconv.patch" Index: lib/libc/iconv/citrus_csmapper.c =================================================================== --- lib/libc/iconv/citrus_csmapper.c (revision 247143) +++ lib/libc/iconv/citrus_csmapper.c (working copy) @@ -312,24 +312,36 @@ get_none(struct _citrus_mapper_area *__restrict ma, struct _citrus_csmapper *__restrict *__restrict rcsm) { + struct _citrus_csmapper *csm_tmp; int ret; - WLOCK; - if (csm_none) { + WLOCK(); + if (csm_none != NULL) { *rcsm = csm_none; + UNLOCK(); ret = 0; goto quit; } - ret = _mapper_open_direct(ma, &csm_none, "mapper_none", ""); + UNLOCK(); + ret = _mapper_open_direct(ma, &csm_tmp, "mapper_none", ""); if (ret) goto quit; + _mapper_set_persistent(csm_none); + WLOCK(); + if (csm_none != NULL) { + UNLOCK(); + _mapper_close(csm_tmp); + } else { + csm_none = csm_tmp; + UNLOCK(); + } + *rcsm = csm_none; ret = 0; quit: - UNLOCK; return (ret); } Index: lib/libc/iconv/citrus_iconv.c =================================================================== --- lib/libc/iconv/citrus_iconv.c (revision 247143) +++ lib/libc/iconv/citrus_iconv.c (working copy) @@ -72,7 +72,7 @@ init_cache(void) { - WLOCK; + WLOCK(); if (!isinit) { _CITRUS_HASH_INIT(&shared_pool, CI_HASH_SIZE); TAILQ_INIT(&shared_unused); @@ -83,7 +83,7 @@ shared_max_reuse = CI_INITIAL_MAX_REUSE; isinit = true; } - UNLOCK; + UNLOCK(); } static __inline void @@ -189,16 +189,16 @@ get_shared(struct _citrus_iconv_shared * __restrict * __restrict rci, const char *src, const char *dst) { - struct _citrus_iconv_shared * ci; + struct _citrus_iconv_shared * ci, *citmp; char convname[PATH_MAX]; int hashval, ret = 0; snprintf(convname, sizeof(convname), "%s/%s", src, dst); - WLOCK; - /* lookup alread existing entry */ hashval = hash_func(convname); + + WLOCK(); _CITRUS_HASH_SEARCH(&shared_pool, ci, ci_hash_entry, match_func, convname, hashval); if (ci != NULL) { @@ -209,21 +209,32 @@ } ci->ci_used_count++; *rci = ci; + UNLOCK(); goto quit; } + UNLOCK(); /* create new entry */ ret = open_shared(&ci, convname, src, dst); if (ret) goto quit; + WLOCK(); + _CITRUS_HASH_SEARCH(&shared_pool, citmp, ci_hash_entry, match_func, + convname, hashval); + if (citmp != NULL) { + UNLOCK(); + close_shared(ci); + ci = citmp; + goto quit; + } + _CITRUS_HASH_INSERT(&shared_pool, ci, ci_hash_entry, hashval); ci->ci_used_count = 1; *rci = ci; + UNLOCK(); quit: - UNLOCK; - return (ret); } @@ -231,7 +242,7 @@ release_shared(struct _citrus_iconv_shared * __restrict ci) { - WLOCK; + WLOCK(); ci->ci_used_count--; if (ci->ci_used_count == 0) { /* put it into unused list */ @@ -243,11 +254,13 @@ TAILQ_REMOVE(&shared_unused, ci, ci_tailq_entry); _CITRUS_HASH_REMOVE(ci, ci_hash_entry); shared_num_unused--; + UNLOCK(); close_shared(ci); + WLOCK(); } } - UNLOCK; + UNLOCK(); } /* @@ -273,6 +286,7 @@ dst = nl_langinfo(CODESET); /* resolve codeset name aliases */ + path[0] = '\x0'; /* XXX - just copy src/dst to realsrc/dst? */ strlcpy(realsrc, _lookup_alias(path, src, buf, (size_t)PATH_MAX, _LOOKUP_CASE_IGNORE), (size_t)PATH_MAX); strlcpy(realdst, _lookup_alias(path, dst, buf, (size_t)PATH_MAX, Index: lib/libc/iconv/citrus_lock.c =================================================================== --- lib/libc/iconv/citrus_lock.c (revision 0) +++ lib/libc/iconv/citrus_lock.c (working copy) @@ -0,0 +1,3 @@ +#include "citrus_lock.h" + +pthread_rwlock_t lock = PTHREAD_RWLOCK_INITIALIZER; Index: lib/libc/iconv/citrus_lock.h =================================================================== --- lib/libc/iconv/citrus_lock.h (revision 247143) +++ lib/libc/iconv/citrus_lock.h (working copy) @@ -27,9 +27,13 @@ #include -static pthread_rwlock_t lock; +extern pthread_rwlock_t lock; -#define WLOCK if (__isthreaded) \ - pthread_rwlock_wrlock(&lock); -#define UNLOCK if (__isthreaded) \ - pthread_rwlock_unlock(&lock); +#define WLOCK() do { \ + if (__isthreaded) \ + pthread_rwlock_wrlock(&lock); \ + } while (0) +#define UNLOCK() do { \ + if (__isthreaded) \ + pthread_rwlock_unlock(&lock); \ + } while (0) Index: lib/libc/iconv/citrus_mapper.c =================================================================== --- lib/libc/iconv/citrus_mapper.c (revision 247143) +++ lib/libc/iconv/citrus_mapper.c (working copy) @@ -75,7 +75,7 @@ char path[PATH_MAX]; int ret; - WLOCK; + WLOCK(); if (*rma != NULL) { ret = 0; @@ -104,7 +104,7 @@ *rma = ma; ret = 0; quit: - UNLOCK; + UNLOCK(); return (ret); } @@ -309,14 +309,14 @@ struct _citrus_mapper * __restrict * __restrict rcm, const char * __restrict mapname) { - struct _citrus_mapper *cm; + struct _citrus_mapper *cm, *cmtmp; char linebuf[PATH_MAX]; const char *module, *variable; int hashval, ret; variable = NULL; - WLOCK; + WLOCK(); /* search in the cache */ hashval = hash_func(mapname); @@ -325,9 +325,11 @@ if (cm) { /* found */ cm->cm_refcount++; + UNLOCK(); +foundwref: *rcm = cm; ret = 0; - goto quit; + goto quitnolock; } /* search mapper entry */ @@ -337,16 +339,28 @@ goto quit; /* open mapper */ - UNLOCK; + UNLOCK(); ret = mapper_open(ma, &cm, module, variable); - WLOCK; if (ret) - goto quit; + goto quitnolock; + + WLOCK(); + _CITRUS_HASH_SEARCH(&ma->ma_cache, cmtmp, cm_entry, match_func, mapname, + hashval); + if (cmtmp) { + cmtmp->cm_refcount++; + UNLOCK(); + _mapper_close(cm); + cm = cmtmp; + goto foundwref; + } + cm->cm_key = strdup(mapname); if (cm->cm_key == NULL) { ret = errno; + UNLOCK(); _mapper_close(cm); - goto quit; + goto quitnolock; } /* insert to the cache */ @@ -356,8 +370,9 @@ *rcm = cm; ret = 0; quit: - UNLOCK; + UNLOCK(); +quitnolock: return (ret); } @@ -369,20 +384,21 @@ _citrus_mapper_close(struct _citrus_mapper *cm) { - if (cm) { - WLOCK; - if (cm->cm_refcount == REFCOUNT_PERSISTENT) - goto quit; - if (cm->cm_refcount > 0) { - if (--cm->cm_refcount > 0) - goto quit; - _CITRUS_HASH_REMOVE(cm, cm_entry); - free(cm->cm_key); - } - mapper_close(cm); -quit: - UNLOCK; + if (cm == NULL) + return; + + WLOCK(); + if (cm->cm_refcount == REFCOUNT_PERSISTENT || --cm->cm_refcount != 0) { + UNLOCK(); + return; } + + _CITRUS_HASH_REMOVE(cm, cm_entry); + free(cm->cm_key); + cm->cm_key = NULL; + + UNLOCK(); + mapper_close(cm); } /* @@ -393,7 +409,7 @@ _citrus_mapper_set_persistent(struct _citrus_mapper * __restrict cm) { - WLOCK; + WLOCK(); cm->cm_refcount = REFCOUNT_PERSISTENT; - UNLOCK; + UNLOCK(); } Index: lib/libc/iconv/citrus_module.c =================================================================== --- lib/libc/iconv/citrus_module.c (revision 247143) +++ lib/libc/iconv/citrus_module.c (working copy) @@ -109,7 +109,6 @@ #include "citrus_namespace.h" #include "citrus_bcs.h" #include "citrus_module.h" -#include "libc_private.h" static int _getdewey(int[], char *); static int _cmpndewey(int[], int, int[], int); @@ -293,11 +292,11 @@ maj = I18NMODULE_MAJOR; min = -1; p = _findshlib(path, &maj, &min); - if (!p) + if (p == NULL) return (EINVAL); - handle = libc_dlopen(p, RTLD_LAZY); - if (!handle) { - printf("%s", dlerror()); + handle = dlopen(p, RTLD_LAZY); + if (handle == NULL) { + printf("%s", dlerror()); /* XXX */ return (EINVAL); } Index: lib/libiconv/Makefile =================================================================== --- lib/libiconv/Makefile (revision 247143) +++ lib/libiconv/Makefile (working copy) @@ -14,11 +14,11 @@ SRCS= citrus_bcs.c citrus_bcs_strtol.c citrus_bcs_strtoul.c \ citrus_csmapper.c citrus_db.c citrus_db_factory.c \ citrus_db_hash.c citrus_esdb.c citrus_hash.c \ - citrus_iconv.c citrus_lookup.c citrus_lookup_factory.c \ + citrus_iconv.c citrus_lock.c citrus_lookup.c citrus_lookup_factory.c \ citrus_mapper.c citrus_memstream.c citrus_mmap.c \ citrus_module.c citrus_none.c citrus_pivot_factory.c \ citrus_prop.c citrus_stdenc.c iconv.c -CFLAGS+= --param max-inline-insns-single=128 -I ${.CURDIR}/../../include -I${.CURDIR}/../libc/include +CFLAGS+= -I ${.CURDIR}/../../include .include --4f28nU6agdXSinmL--