Re: [PATCH,HURD] Add SysV SHM support

[Roland McGrath <roland at hack dot frob dot com>]

I can't see how this patch could apply to the trunk code.
Please submit a patch that is verified to do so.

> 2005-07-11  Marcus Brinkmann  <marcus@gnu.org>

Note for the actual commit: we now use the date of merge rather than the
original date in the ChangeLog entry.

>         * hurd/Makefile (routines): Add sysvshm.
>         (distribute): Add sysvshm.h.

As Joseph mentioned, distribute is long obsolete.

>         * sysdeps/mach/hurd/bits/stat.h (S_IMMAP0): New macro.
>         (S_ISPARE): Unset the S_IMMAP0 flag.

This change is missing from the patch.

> --- /dev/null
> +++ b/hurd/sysvshm.c
> @@ -0,0 +1,96 @@
> +/* Copyright (C) 2005 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.

Start every new file with a descriptive comment and put the copyright on
the second line.  Use the current year for a new file.

> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, write to the Free
> +   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> +   02111-1307 USA.  */

Use the new canonical copyright text you find in other libc files.

> +/* List of attachments.  */
> +static struct sysvshm_attach *attach_list;

Even though it's static, it's helpful to give this a more descriptive
name so it's obvious outside this context what it's for, e.g. "sysvshm_list".

> +  shm = malloc (sizeof (*shm));u
> +  if (!shm)

Write (shm == NULL).

> +/* Removes a segment attachment.  Returns its size if found, or EINVAL
> +   otherwise.  */

On success, returns 0 and sets *SIZE to its size.
Returns EINVAL if not found.

> +  while (shm)

Write (shm != NULL).

> --- /dev/null
> +++ b/hurd/sysvshm.h
> @@ -0,0 +1,47 @@

Same issues with the initial comment, copyright year and text.

> +#include <paths.h>
> +#include <hurd.h>

The header might as well have a multiple-inclusion guard even if it's
not really necessary.

> +/* The area (from top to bottom) that is used for private keys.  These
> +   are all keys that have the second highest bit set.  */
> +#define SHM_PRIV_KEY_START INT_MAX
> +#define SHM_PRIV_KEY_END ((INT_MAX / 2) + 1)
> +
> +#define SHM_PREFIX "shm-"
> +#define SHM_DIR _PATH_DEV "shm/"

Use tabs so the rhs of all these is aligned.

This file name space conflicts with that used for shm_open.
It's probably OK to use space under /dev/shm for sysvhm too,
but make the prefix "sysvshm-".

> diff --git a/sysdeps/mach/hurd/ftok.c b/sysdeps/mach/hurd/ftok.c
> new file mode 100644
> index 0000000..8d8b5cb
> --- /dev/null
> +++ b/sysdeps/mach/hurd/ftok.c
> @@ -0,0 +1,43 @@
> +/* Copyright (C) 1995, 1996, 2000, 2005 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Ulrich Drepper <drepper@gnu.ai.mit.edu>, August 1995.

This function is trivial enough that there is no need to preserve
the old copyright years and "Contributed by" line from sysvipc/ftok.c.

> +/* In the Hurd, we use the second-to-most-significant bit as flag for
> +   private keys.  We use a different order of the components so that
> +   the biggest one---the inode number---is affected by this.  */

A long dash in a comment is -- not ---.

When you say "different" here it's not very obvious what baseline
you're referring to.  Say "different from the generic code in
sysvipc/ftok.c" or something like that.

> +  err = __sysvshm_add (addr, statbuf.st_size);
> +  if (err)
> +    {
> +      munmap (addr, statbuf.st_size);
> +      return (void *) -1;
> +    }

Set errno here.

> +/* Provide operations to control over shared memory segments.  */

s/control over/control/

> +__shmctl (int id, int cmd, struct shmid_ds *buf)
> +{
> +  error_t err = 0;
> +  int fd;
> +  int res;
> +  char filename[sizeof (SHM_DIR) - 1 + SHM_NAMEMAX];
> +  struct stat statbuf;
> +
> +  sprintf (filename, SHM_DIR SHM_NAMEPRI, id);
> +  /* SysV requires read access for IPC_STAT.  */
> +  fd = __open (filename, O_NORW);
> +  if (fd < 0)
> +    {
> +      if (errno == ENOENT)
> +	errno = EINVAL;
> +      return -1;
> +    }

Since this is repeated in more than one function, put it into an
internal subroutine.  Then we have only one place doing the
name-generation logic.

> +    case IPC_RMID:
> +      res = __unlink (filename);
> +      /* FIXME: Check error (mapping ENOENT to EINVAL).  */

Fix it.


Thanks,
Roland