[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH,HURD] Add SysV SHM support
From: |
Roland McGrath |
Subject: |
Re: [PATCH,HURD] Add SysV SHM support |
Date: |
Mon, 14 May 2012 12:16:41 -0700 (PDT) |
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