From patchwork Thu Dec 4 19:37:10 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Roland McGrath X-Patchwork-Id: 417893 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id D9C271400B7 for ; Fri, 5 Dec 2014 06:37:20 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:content-type :content-transfer-encoding:from:to:subject:in-reply-to :references:message-id:date; q=dns; s=default; b=PLvG3XU4WUz1P8t qf+NQDQdIuqwBie+m9L2lugKge/pJlQ6VucFE+72icOA/LNWx+r9QwRmws4kyoCC Jc7sy5QeK3JOrq58h47M0V4vtfGWQBcNiWyr+UkNS2iWbFIgRtZTD/hSsW9Mm0Wg 286taA4JwE8F3LXGgHtZU93uudps= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:content-type :content-transfer-encoding:from:to:subject:in-reply-to :references:message-id:date; s=default; bh=kQ8db3MezwYVcSJPnJbTU 3W418o=; b=YfUxu1unDUVsYfaGVF50RIWG2BKeQv84d8p+DBOKMJe/QS55PhTrg pbx7PiC7O7saE3E56p4/aj2qUtycmV8RTjggQ8eUG+IcPp3G1gUR7B0ji2kopJjr UboKHD3mV7FcrwRCGZPUWN60B56kKzqBB0FJXyLppY3CLZ7OGUeOIw= Received: (qmail 11311 invoked by alias); 4 Dec 2014 19:37:14 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 11300 invoked by uid 89); 4 Dec 2014 19:37:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 X-HELO: topped-with-meat.com MIME-Version: 1.0 From: Roland McGrath To: "GNU C. Library" Subject: [PATCH 2/2 roland/namedsem] NPTL: Refactor named semaphore code to use shm-directory.h In-Reply-To: Roland McGrath's message of Thursday, 4 December 2014 11:29:39 -0800 <20141204192939.9EE322C3A08@topped-with-meat.com> References: <20141204192939.9EE322C3A08@topped-with-meat.com> Message-Id: <20141204193710.6F1E02C3A08@topped-with-meat.com> Date: Thu, 4 Dec 2014 11:37:10 -0800 (PST) X-CMAE-Score: 0 X-CMAE-Analysis: v=2.1 cv=SvUDtp+0 c=1 sm=1 tr=0 a=WkljmVdYkabdwxfqvArNOQ==:117 a=14OXPxybAAAA:8 a=kj9zAlcOel0A:10 a=hOe2yjtxAAAA:8 a=bUayKFOu97xLwXad5aAA:9 a=CjuIK1q_8ugA:10 This builds on the previous patch to remove duplication and Linuxism from the sem_open/sem_unlink implementation. There was massive duplication of the logic to deduce the directory name that's usually /dev/shm/, between the shm_open/shm_unlink code and the sem_open/sem_unlink code. That is now in the common shm-directory.h interface used by both. With that factored out, there was no longer any nontrivial Linuxism in sem_open, so I also removed the trivial ones. This moves the shm-directory code from librt to libpthread, since sem_open is in libpthread and so needs it there. librt already depends on libpthread, so it can just use the function from there. For non-NPTL configurations, it remains in librt. One tiny change in behavior is introduced by coalescing the name-checking logic together between shm_open and sem_open. shm_open refuses any name with a '/' in it, and not sem_open does too. Previously it wouldn't diagnose EINVAL for that case, but the name "foo/bar" would always fail with ENOENT unless someone had done 'mkdir /dev/shm/sem.foo/' already. I think the lack of checking was just an oversight, so I am not considering this any kind of ABI change. Tested x86_64-linux-gnu. If I don't hear any feedback by the end of the week, I'll commit this early next week. But I'd appreciate some review. Thanks, Roland * sysdeps/posix/shm-directory.h (SHM_GET_NAME): Take new argument PREFIX, string constant to insert between directory and name. * sysdeps/posix/shm_open.c: Update caller. * sysdeps/posix/shm_unlink.c: Likewise. * nptl/semaphoreP.h (struct mountpoint_info): Type removed. (__where_is_shmfs, mountpoint, __namedsem_once): Declarations removed. (SEM_SHM_PREFIX): New macro. * sysdeps/posix/Makefile (librt-routines): Add shm-directory only if [$(have-thread-library) = no]. * nptl/Makefile (libpthread-routines): Add shm-directory. * nptl/Versions (GLIBC_PRIVATE): Add __shm_directory. * nptl/sem_open.c (check_add_mapping): Use munmap function rather than INTERNAL_SYSCALL. (__where_is_shmfs): Function removed. (mountpoint, defaultmount, defaultdir, __namedsem_once): Variables removed. (sem_open): Use close function rather than INTERNAL_SYSCALL. Use SHM_GET_NAME. * nptl/sem_unlink.c: Prototypify. Use SHM_GET_NAME. --- a/nptl/Makefile +++ b/nptl/Makefile @@ -96,6 +96,7 @@ libpthread-routines = nptl-init vars events version \ old_pthread_atfork pthread_atfork \ pthread_getcpuclockid \ pthread_clock_gettime pthread_clock_settime \ + shm-directory \ sem_init sem_destroy \ sem_open sem_close sem_unlink \ sem_getvalue \ --- a/nptl/Versions +++ b/nptl/Versions @@ -270,5 +270,6 @@ libpthread { __pthread_initialize_minimal; __pthread_clock_gettime; __pthread_clock_settime; __pthread_unwind; __pthread_get_minstack; + __shm_directory; } } --- a/nptl/sem_open.c +++ b/nptl/sem_open.c @@ -18,8 +18,6 @@ #include #include -#include -#include #include #include #include @@ -30,98 +28,8 @@ #include #include #include -#include -#include #include "semaphoreP.h" - - - -/* Information about the mount point. */ -struct mountpoint_info mountpoint attribute_hidden; - -/* This is the default mount point. */ -static const char defaultmount[] = "/dev/shm"; -/* This is the default directory. */ -static const char defaultdir[] = "/dev/shm/sem."; - -/* Protect the `mountpoint' variable above. */ -pthread_once_t __namedsem_once attribute_hidden = PTHREAD_ONCE_INIT; - - -/* Determine where the shmfs is mounted (if at all). */ -void -attribute_hidden -__where_is_shmfs (void) -{ - char buf[512]; - struct statfs f; - struct mntent resmem; - struct mntent *mp; - FILE *fp; - - /* The canonical place is /dev/shm. This is at least what the - documentation tells everybody to do. */ - if (__statfs (defaultmount, &f) == 0 && (f.f_type == SHMFS_SUPER_MAGIC - || f.f_type == RAMFS_MAGIC)) - { - /* It is in the normal place. */ - mountpoint.dir = (char *) defaultdir; - mountpoint.dirlen = sizeof (defaultdir) - 1; - - return; - } - - /* OK, do it the hard way. Look through the /proc/mounts file and if - this does not exist through /etc/fstab to find the mount point. */ - fp = __setmntent ("/proc/mounts", "r"); - if (__glibc_unlikely (fp == NULL)) - { - fp = __setmntent (_PATH_MNTTAB, "r"); - if (__glibc_unlikely (fp == NULL)) - /* There is nothing we can do. Blind guesses are not helpful. */ - return; - } - - /* Now read the entries. */ - while ((mp = __getmntent_r (fp, &resmem, buf, sizeof buf)) != NULL) - /* The original name is "shm" but this got changed in early Linux - 2.4.x to "tmpfs". */ - if (strcmp (mp->mnt_type, "tmpfs") == 0 - || strcmp (mp->mnt_type, "shm") == 0) - { - /* Found it. There might be more than one place where the - filesystem is mounted but one is enough for us. */ - size_t namelen; - - /* First make sure this really is the correct entry. At least - some versions of the kernel give wrong information because - of the implicit mount of the shmfs for SysV IPC. */ - if (__statfs (mp->mnt_dir, &f) != 0 || (f.f_type != SHMFS_SUPER_MAGIC - && f.f_type != RAMFS_MAGIC)) - continue; - - namelen = strlen (mp->mnt_dir); - - if (namelen == 0) - /* Hum, maybe some crippled entry. Keep on searching. */ - continue; - - mountpoint.dir = (char *) malloc (namelen + 4 + 2); - if (mountpoint.dir != NULL) - { - char *cp = __mempcpy (mountpoint.dir, mp->mnt_dir, namelen); - if (cp[-1] != '/') - *cp++ = '/'; - cp = stpcpy (cp, "sem."); - mountpoint.dirlen = cp - mountpoint.dir; - } - - break; - } - - /* Close the stream. */ - __endmntent (fp); -} +#include /* Comparison function for search of existing mapping. */ @@ -217,8 +125,9 @@ check_add_mapping (const char *name, size_t namelen, int fd, sem_t *existing) if (result != existing && existing != SEM_FAILED && existing != MAP_FAILED) { /* Do not disturb errno. */ - INTERNAL_SYSCALL_DECL (err); - INTERNAL_SYSCALL (munmap, err, 2, existing, sizeof (sem_t)); + int save = errno; + munmap (existing, sizeof (sem_t)); + errno = save; } return result; @@ -228,42 +137,17 @@ check_add_mapping (const char *name, size_t namelen, int fd, sem_t *existing) sem_t * sem_open (const char *name, int oflag, ...) { - char *finalname; - sem_t *result = SEM_FAILED; int fd; + sem_t *result; - /* Determine where the shmfs is mounted. */ - __pthread_once (&__namedsem_once, __where_is_shmfs); - - /* If we don't know the mount points there is nothing we can do. Ever. */ - if (mountpoint.dir == NULL) - { - __set_errno (ENOSYS); - return SEM_FAILED; - } - - /* Construct the filename. */ - while (name[0] == '/') - ++name; - - if (name[0] == '\0') - { - /* The name "/" is not supported. */ - __set_errno (EINVAL); - return SEM_FAILED; - } - size_t namelen = strlen (name) + 1; - - /* Create the name of the final file. */ - finalname = (char *) alloca (mountpoint.dirlen + namelen); - __mempcpy (__mempcpy (finalname, mountpoint.dir, mountpoint.dirlen), - name, namelen); + /* Create the name of the final file in local variable SHM_NAME. */ + SHM_GET_NAME (EINVAL, SEM_FAILED, SEM_SHM_PREFIX); /* If the semaphore object has to exist simply open it. */ if ((oflag & O_CREAT) == 0 || (oflag & O_EXCL) == 0) { try_again: - fd = __libc_open (finalname, + fd = __libc_open (shm_name, (oflag & ~(O_CREAT|O_ACCMODE)) | O_NOFOLLOW | O_RDWR); if (fd == -1) @@ -317,8 +201,8 @@ sem_open (const char *name, int oflag, ...) memset ((char *) &sem.initsem + sizeof (struct new_sem), '\0', sizeof (sem_t) - sizeof (struct new_sem)); - tmpfname = (char *) alloca (mountpoint.dirlen + 6 + 1); - char *xxxxxx = __mempcpy (tmpfname, mountpoint.dir, mountpoint.dirlen); + tmpfname = __alloca (shm_dirlen + sizeof SEM_SHM_PREFIX + 6); + char *xxxxxx = __mempcpy (tmpfname, shm_dir, shm_dirlen); int retries = 0; #define NRETRIES 50 @@ -361,7 +245,7 @@ sem_open (const char *name, int oflag, ...) fd, 0)) != MAP_FAILED) { /* Create the file. Don't overwrite an existing file. */ - if (link (tmpfname, finalname) != 0) + if (link (tmpfname, shm_name) != 0) { /* Undo the mapping. */ (void) munmap (result, sizeof (sem_t)); @@ -402,8 +286,9 @@ sem_open (const char *name, int oflag, ...) if (fd != -1) { /* Do not disturb errno. */ - INTERNAL_SYSCALL_DECL (err); - INTERNAL_SYSCALL (close, err, 1, fd); + int save = errno; + close (fd); + errno = save; } return result; --- a/nptl/sem_unlink.c +++ b/nptl/sem_unlink.c @@ -22,44 +22,16 @@ #include #include #include "semaphoreP.h" - +#include int -sem_unlink (name) - const char *name; +sem_unlink (const char *name) { - char *fname; - size_t namelen; - - /* Determine where the shmfs is mounted. */ - __pthread_once (&__namedsem_once, __where_is_shmfs); - - /* If we don't know the mount points there is nothing we can do. Ever. */ - if (mountpoint.dir == NULL) - { - __set_errno (ENOSYS); - return -1; - } - /* Construct the filename. */ - while (name[0] == '/') - ++name; - - if (name[0] == '\0') - { - /* The name "/" is not supported. */ - __set_errno (ENOENT); - return -1; - } - namelen = strlen (name); - - /* Create the name of the file. */ - fname = (char *) alloca (mountpoint.dirlen + namelen + 1); - __mempcpy (__mempcpy (fname, mountpoint.dir, mountpoint.dirlen), - name, namelen + 1); + SHM_GET_NAME (ENOENT, -1, SEM_SHM_PREFIX); /* Now try removing it. */ - int ret = unlink (fname); + int ret = unlink (shm_name); if (ret < 0 && errno == EPERM) __set_errno (EACCES); return ret; --- a/nptl/semaphoreP.h +++ b/nptl/semaphoreP.h @@ -19,13 +19,7 @@ #include #include "pthreadP.h" - -/* Mount point of the shared memory filesystem. */ -struct mountpoint_info -{ - char *dir; - size_t dirlen; -}; +#define SEM_SHM_PREFIX "sem." /* Keeping track of currently used mappings. */ struct inuse_sem @@ -38,11 +32,6 @@ struct inuse_sem }; -/* Variables used in multiple interfaces. */ -extern struct mountpoint_info mountpoint attribute_hidden; - -extern pthread_once_t __namedsem_once attribute_hidden; - /* The search tree for existing mappings. */ extern void *__sem_mappings attribute_hidden; @@ -50,9 +39,6 @@ extern void *__sem_mappings attribute_hidden; extern int __sem_mappings_lock attribute_hidden; -/* Initializer for mountpoint. */ -extern void __where_is_shmfs (void) attribute_hidden; - /* Comparison function for search in tree with existing mappings. */ extern int __sem_search (const void *a, const void *b) attribute_hidden; --- a/sysdeps/posix/Makefile +++ b/sysdeps/posix/Makefile @@ -4,6 +4,8 @@ TMP_MAX = 238328 L_ctermid = 9 L_cuserid = 9 -ifeq ($(subdir),rt) +ifeq ($(subdir)|$(have-thread-library),rt|no) +# With NPTL, this lives in libpthread so it can be used for sem_open too. +# Without NPTL, it's just private in librt. librt-routines += shm-directory endif --- a/sysdeps/posix/shm-directory.h +++ b/sysdeps/posix/shm-directory.h @@ -36,9 +36,10 @@ extern const char *__shm_directory (size_t *len); strlen (NAME) + 1. If NAME is invalid, it sets errno to ERRNO_FOR_INVALID and returns RETVAL_FOR_INVALID. Finally, it defines the local variable SHM_NAME, giving the absolute file name of the shm - file corresponding to NAME. */ + file corresponding to NAME. PREFIX is a string constant used as a + prefix on NAME. */ -#define SHM_GET_NAME(errno_for_invalid, retval_for_invalid) \ +#define SHM_GET_NAME(errno_for_invalid, retval_for_invalid, prefix) \ size_t shm_dirlen; \ const char *shm_dir = __shm_directory (&shm_dirlen); \ /* If we don't know what directory to use, there is nothing we can do. */ \ @@ -57,7 +58,9 @@ extern const char *__shm_directory (size_t *len); __set_errno (errno_for_invalid); \ return retval_for_invalid; \ } \ - char *shm_name = __alloca (shm_dirlen + namelen); \ - __mempcpy (__mempcpy (shm_name, shm_dir, shm_dirlen), name, namelen) + char *shm_name = __alloca (shm_dirlen + sizeof prefix - 1 + namelen); \ + __mempcpy (__mempcpy (__mempcpy (shm_name, shm_dir, shm_dirlen), \ + prefix, sizeof prefix - 1), \ + name, namelen) #endif /* shm-directory.h */ --- a/sysdeps/posix/shm_open.c +++ b/sysdeps/posix/shm_open.c @@ -32,7 +32,7 @@ int shm_open (const char *name, int oflag, mode_t mode) { - SHM_GET_NAME (EINVAL, -1); + SHM_GET_NAME (EINVAL, -1, ""); # ifdef O_NOFOLLOW oflag |= O_NOFOLLOW; --- a/sysdeps/posix/shm_unlink.c +++ b/sysdeps/posix/shm_unlink.c @@ -32,7 +32,7 @@ int shm_unlink (const char *name) { - SHM_GET_NAME (ENOENT, -1); + SHM_GET_NAME (ENOENT, -1, ""); int result = unlink (shm_name); if (result < 0 && errno == EPERM)