[2/2,roland/namedsem] NPTL: Refactor named semaphore code to use shm-directory.h
diff mbox

Message ID 20141204193710.6F1E02C3A08@topped-with-meat.com
State New
Headers show

Commit Message

Roland McGrath Dec. 4, 2014, 7:37 p.m. UTC
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.

Patch
diff mbox

--- 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 <errno.h>
 #include <fcntl.h>
-#include <mntent.h>
-#include <paths.h>
 #include <pthread.h>
 #include <search.h>
 #include <semaphore.h>
@@ -30,98 +28,8 @@ 
 #include <unistd.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
-#include <sys/statfs.h>
-#include <linux_fsinfo.h>
 #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 <shm-directory.h>
 
 
 /* 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 <string.h>
 #include <unistd.h>
 #include "semaphoreP.h"
-
+#include <shm-directory.h>
 
 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 <semaphore.h>
 #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)