Re: [PATCH,HURD] Add SysV SHM support

[Kalle Olavi Niemitalo <kon at iki dot fi>]

Samuel Thibault <samuel.thibault@gnu.org> writes:

> +/* Removes a segment attachment.  Returns its size if found, or EINVAL
> +   otherwise.  */
> +error_t
> +__sysvshm_remove (void *addr, size_t *size)

Doesn't literally return the size.  There is a more accurate
comment in sysvshm.h; perhaps this one could be just removed.

In that function:

> +      if (shm->addr == addr)
> +        {
> +          *pshm = shm->next;
> +          *size = shm->size;
> +          __mutex_unlock (&sysvshm_lock);
> +          return 0;
> +        }

Shouldn't this free (shm)?  It seems to leak otherwise.

In sysdeps/mach/hurd/shmat.c (__shmat):

> +  res = __fstat (fd, &statbuf);
> +  if (res < 0)
> +    {
> +      __close (fd);
> +      return (void *) -1;
> +    }

Why doesn't this save and restore errno across __close like
__shmctl does?  I expect it's necessary in both or in neither.

In sysdeps/mach/hurd/shmctl.c (__shmctl):

> +      buf->shm_perm.__key = id;

If id refers to a shared-memory segment created with key ==
IPC_PRIVATE, then shmctl IPC_STAT should set buf->shm_perm.__key
= IPC_PRIVATE.  At least, that's what happens under GNU/Linux.
It seems this would be easy to implement in __shmctl.

In sysdeps/mach/hurd/shmdt.c (__shmdt):

> +  __munmap ((void *) shmaddr, size);
> +  return 0;

I wonder if this should check for errors from __munmap.

In sysdeps/mach/hurd/shmget.c (get_exclusive):

> +      /* Try to link the shared memory segment into the filesystem
> +	 (exclusively).  Private segments have negative keys.  */

They don't have negative keys.  Both SHM_PRIV_KEY_START and
SHM_PRIV_KEY_END are positive.

Attachment: pgp00000.pgp
Description: PGP signature