Re: [PATCH 1/3] Reload fd ioctl handler on each call to ioctl

[olafBuddenhagen]

Hi,

First of all, I think the title of the patch is wrong. The relevant bit
is that this patch has the initial implementation for loading
server-provided ioctl handlers. The fact that they are reloaded each
time, is something that would normally be mentioned in the commit
message; but certainly not in the title.

On Wed, Aug 26, 2009 at 04:42:32PM +0200, Carl Fredrik Hammar wrote:

> * hurd/hurdioctl.c (_hurd_dummy_ioctl_handler): New function.
> * hurd/hurd/ioctl.h (_hurd_dummy_ioctl_handler): Likewise.
> * hurd/fd-ioctl-call.c: New file.
> * hurd/hurd/fd.h: Update copyright years.

Don't mention this in the changelog.

> (_hurd_fd_call_ioctl_handler): New function declaration.
> * hurd/Makefile: Update copyright years.
> (user-interfaces): Add `ioctl_handler'.
> (dtable): Add `fd-ioctl-call'.
> * sysdeps/mach/hurd/ioctl.c: Update copyright years.
> (__ioctl): Call fd ioctl handler.
> ---
>  hurd/Makefile             |    7 ++-
>  hurd/fd-ioctl-call.c      |   90 
> +++++++++++++++++++++++++++++++++++++++++++++
>  hurd/hurd/fd.h            |    6 ++-
>  hurd/hurd/ioctl.h         |    5 ++
>  hurd/hurdioctl.c          |    9 ++++-
>  sysdeps/mach/hurd/ioctl.c |   12 +++++-
>  6 files changed, 123 insertions(+), 6 deletions(-)
>  create mode 100644 hurd/fd-ioctl-call.c
> 
> diff --git a/hurd/Makefile b/hurd/Makefile
> index ff6b7cb..4ad5128 100644
> --- a/hurd/Makefile
> +++ b/hurd/Makefile
> @@ -1,4 +1,4 @@
> -# Copyright (C) 1991,92,93,94,95,96,97,98,99,2001,2002,2004,2006
> +# Copyright (C) 1991,92,93,94,95,96,97,98,99,2001,2002,2004,2006,2009
>  #    Free Software Foundation, Inc.
>  # This file is part of the GNU C Library.
>  
> @@ -40,7 +40,7 @@ user-interfaces             := $(addprefix hurd/,\
>                                      msg msg_reply msg_request \
>                                      exec exec_startup crash interrupt \
>                                      fs fsys io term tioctl socket ifsock \
> -                                    login password pfinet \
> +                                    login password pfinet ioctl_handler \
>                                      )
>  server-interfaces    := hurd/msg faultexc
>  
> @@ -67,7 +67,8 @@ sig = hurdsig hurdfault siginfo hurd-raise preempt-sig \
>         thread-self thread-cancel intr-msg catch-signal
>  dtable       = dtable port2fd new-fd alloc-fd intern-fd \
>         getdport openport \
> -       fd-close fd-read fd-write hurdioctl ctty-input ctty-output
> +       fd-close fd-read fd-write hurdioctl ctty-input ctty-output \
> +       fd-ioctl-call
>  inlines = $(inline-headers:%.h=%-inlines)
>  distribute = hurdstartup.h hurdfault.h hurdhost.h sysvshm.h \
>            faultexc.defs intr-rpc.defs intr-rpc.h intr-msg.h Notes
> diff --git a/hurd/fd-ioctl-call.c b/hurd/fd-ioctl-call.c
> new file mode 100644
> index 0000000..c5a41e8
> --- /dev/null
> +++ b/hurd/fd-ioctl-call.c
> @@ -0,0 +1,90 @@
> +/* Call descriptors ioctl handler.
> +   Copyright (C) 2009 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   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., 51 Franklin Street, Fifth Floor,
> +   Boston, MA 02110-1301, USA.  */
> +
> +#include <hurd.h>
> +#include <hurd/fd.h>
> +#include <hurd/ioctl.h>
> +#include <hurd/ioctl_handler.h>
> +#include <dlfcn.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +
> +
> +/* Get PORT's ioctl handler module and load it, returning the linker map
> +   in MAP as returned by `dlopen'.  */
> +
> +static error_t
> +load_ioctl_handler (io_t port, void **map)
> +{
> +  io_t handler;
> +  error_t err;
> +
> +  err = __ioctl_handler_get (port, &handler);
> +  if (!err)
> +    {
> +      int fd = _hurd_intern_fd (handler, 0, 1);  /* Consumes HANDLER.  */
> +      if (fd == -1)
> +     err = errno;
> +      else
> +     {
> +       char *name;
> +       err = __asprintf (&name, "/dev/fd/%d", fd);
> +       if (err == -1)
> +         err = errno;
> +       else
> +         {
> +           *map = __libc_dlopen (name);
> +           free (name);
> +           err = 0;
> +         }
> +       __close (fd);
> +     }
> +    }
> +
> +  return err;
> +}
> +
> +
> +/* Call D's ioctl handler, loading it from the underlying port if
> +   necessary.  Arguments are the same as ioctl handlers.  */
> +
> +int
> +_hurd_fd_call_ioctl_handler (int fd, int request, void *arg)
> +{
> +  ioctl_handler_t ioctl_handler;
> +  void *ioctl_handler_map;
> +  int result;
> +  error_t err;
> +
> +  /* Avoid spurious "may be used uninitialized" warning.  */
> +  ioctl_handler_map = NULL;
> +
> +  err = HURD_DPORT_USE (fd, load_ioctl_handler (port, &ioctl_handler_map));
> +  if (!err && ioctl_handler_map)
> +    ioctl_handler = __libc_dlsym (ioctl_handler_map, "hurd_ioctl_handler");

The "hurd_" prefix is not very descriptive IMHO.

> +  if (err || !ioctl_handler_map || !ioctl_handler)
> +    ioctl_handler = _hurd_dummy_ioctl_handler;
> +
> +  result = (*ioctl_handler) (fd, request, arg);

Using a dummy handler here seems overkill...

> +
> +  if (ioctl_handler_map)
> +    __libc_dlclose (ioctl_handler_map);
> +
> +  return result;
> +}
> diff --git a/hurd/hurd/fd.h b/hurd/hurd/fd.h
> index 2a981f4..6ea9ad6 100644
> --- a/hurd/hurd/fd.h
> +++ b/hurd/hurd/fd.h
> @@ -1,6 +1,6 @@
>  /* File descriptors.
>     Copyright (C) 1993,1994,1995,1996,1997,1998,1999,2000,2001,2002,2006,2007
> -     Free Software Foundation, Inc.
> +     2009 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>  
>     The GNU C Library is free software; you can redistribute it and/or
> @@ -291,5 +291,9 @@ extern file_t __directory_name_split_at (int fd, const 
> char *directory_name,
>                                        char **name);
>  
>  
> +/* Call D's ioctl handler, loading it from the underlying port if
> +   necessary.  Arguments are the same as ioctl handlers.  */
> +extern int _hurd_fd_call_ioctl_handler (int fd, int request, void *arg);
> +
>  
>  #endif       /* hurd/fd.h */
> diff --git a/hurd/hurd/ioctl.h b/hurd/hurd/ioctl.h
> index e5ab3dc..f932fa1 100644
> --- a/hurd/hurd/ioctl.h
> +++ b/hurd/hurd/ioctl.h
> @@ -74,4 +74,9 @@ extern int hurd_register_ioctl_handler (int first_request, 
> int last_request,
>  ioctl_handler_t _hurd_lookup_ioctl_handler (int request);
>  
>  
> +/* A dummy handler that always fails.  */
> +
> +int _hurd_dummy_ioctl_handler (int fd, int request, void *arg);
> +
> +
>  #endif       /* hurd/ioctl.h */
> diff --git a/hurd/hurdioctl.c b/hurd/hurdioctl.c
> index 13a1a78..0b91ee1 100644
> --- a/hurd/hurdioctl.c
> +++ b/hurd/hurdioctl.c
> @@ -1,5 +1,5 @@
>  /* ioctl commands which must be done in the C library.
> -   Copyright (C) 1994,95,96,97,99,2001,02 Free Software Foundation, Inc.
> +   Copyright (C) 1994,95,96,97,99,2001,02,09 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>  
>     The GNU C Library is free software; you can redistribute it and/or
> @@ -50,6 +50,13 @@ _hurd_lookup_ioctl_handler (int request)
>    return NULL;
>  }
>  
> +/* A dummy handler that always fails.  */
> +int
> +_hurd_dummy_ioctl_handler (int fd, int request, void *arg)
> +{
> +  return __hurd_fail (ENOTTY);
> +}
> +
>  #include <fcntl.h>
>  
>  /* Find out how many bytes may be read from FD without blocking.  */
> diff --git a/sysdeps/mach/hurd/ioctl.c b/sysdeps/mach/hurd/ioctl.c
> index 769eb68..736ad8e 100644
> --- a/sysdeps/mach/hurd/ioctl.c
> +++ b/sysdeps/mach/hurd/ioctl.c
> @@ -1,4 +1,4 @@
> -/* Copyright (C) 1992,93,94,95,96,97,99,2000,2002,2005
> +/* Copyright (C) 1992,93,94,95,96,97,99,2000,2002,2005,2009
>       Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>  
> @@ -220,6 +220,16 @@ __ioctl (int fd, unsigned long int request, ...)
>      }
>  
>    {
> +    int save = errno;
> +    int result = _hurd_fd_call_ioctl_handler (fd, request, arg);
> +    if (result != -1 || errno != ENOTTY)
> +      return result;
> +
> +    /* The handler doesn't grok this one.  Try linked handlers.  */
> +    errno = save;
> +  }

I think you should add a comment to this code block.

-antrik-