Re: [PATCH] Use the new __hurd_exec_file_name RPC

[Carl Fredrik Hammar]

Hi,

On Sat, May 22, 2010 at 06:26:29PM +0200, Emilio Pozuelo Monfort wrote:
> diff --git a/hurd/Versions b/hurd/Versions
> index 83c8ab1..81cd904 100644
> --- a/hurd/Versions
> +++ b/hurd/Versions
> @@ -73,6 +73,7 @@ libc {
>      _hurd_critical_section_unlock;
>      _hurd_exception2signal;
>      _hurd_exec;
> +    _hurd_exec_file_name;
>      _hurd_fd_get;
>      _hurd_init;
>      _hurd_intern_fd;

Perhaps you missed it in the confusion but this should go into a new section.
Like this:

  GLIBC_2.12.1 {
    # "quasi-internal" functions
    _hurd_exec_file_name;
  }

Or whatever version this gets into.  You should probably note this in
the commit message so it isn't forgotten.

> diff --git a/hurd/hurdexec.c b/hurd/hurdexec.c
> index beae869..fea2618 100644
> --- a/hurd/hurdexec.c
> +++ b/hurd/hurdexec.c
> @@ -362,13 +377,26 @@ _hurd_exec (task_t task, file_t file,
>        if (__sigismember (&_hurdsig_traced, SIGKILL))
>       flags |= EXEC_SIGTRAP;
>  #endif
> -      err = __file_exec (file, task, flags,
> -                      args, argslen, env, envlen,
> -                      dtable, MACH_MSG_TYPE_COPY_SEND, dtablesize,
> -                      ports, MACH_MSG_TYPE_COPY_SEND, _hurd_nports,
> -                      ints, INIT_INT_MAX,
> -                      please_dealloc, pdp - please_dealloc,
> -                      &_hurd_msgport, task == __mach_task_self () ? 1 : 0);
> +      err = __file_exec_file_name (file, task, flags,
> +                                filename ? filename : "",
> +                                args, argslen, env, envlen,
> +                                dtable, MACH_MSG_TYPE_COPY_SEND, dtablesize,
> +                                ports, MACH_MSG_TYPE_COPY_SEND, _hurd_nports,

Break this line.

> +                                ints, INIT_INT_MAX,
> +                                please_dealloc, pdp - please_dealloc,
> +                                &_hurd_msgport,
> +                                task == __mach_task_self () ? 1 : 0);
> +      /* Fall back for backwards compatibility.  This can just be removed
> +         when __file_exec goes away.  */
> +      if (err)
> +     err = __file_exec (file, task, flags,
> +                        args, argslen, env, envlen,
> +                        dtable, MACH_MSG_TYPE_COPY_SEND, dtablesize,
> +                        ports, MACH_MSG_TYPE_COPY_SEND, _hurd_nports,
> +                        ints, INIT_INT_MAX,
> +                        please_dealloc, pdp - please_dealloc,
> +                        &_hurd_msgport,
> +                        task == __mach_task_self () ? 1 : 0);
>      }
>  
>    /* Release references to the standard ports.  */

Only fall back on MIG_BAD_ID.

> diff --git a/sysdeps/mach/hurd/execve.c b/sysdeps/mach/hurd/execve.c
> index 8af8b33..7875c3c 100644
> --- a/sysdeps/mach/hurd/execve.c
> +++ b/sysdeps/mach/hurd/execve.c
> @@ -1,4 +1,4 @@
> -/* Copyright (C) 1991, 92, 93, 94, 95, 97 Free Software Foundation, Inc.
> +/* Copyright (C) 1991, 92, 93, 94, 95, 97, 2010 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

Break the line.

>     The GNU C Library is free software; you can redistribute it and/or
> @@ -26,8 +26,8 @@
>  int
>  fexecve (int fd, char *const argv[], char *const envp[])
>  {
> -  error_t err = HURD_DPORT_USE (fd, _hurd_exec (__mach_task_self (), port,
> -                                             argv, envp));
> +  error_t err = HURD_DPORT_USE (fd, _hurd_exec_file_name (__mach_task_self 
> (), port,
> +                                                       NULL, argv, envp));

Break these lines even more. :-)

> diff --git a/sysdeps/mach/hurd/spawni.c b/sysdeps/mach/hurd/spawni.c
> index 244ca2d..06728a1 100644
> --- a/sysdeps/mach/hurd/spawni.c
> +++ b/sysdeps/mach/hurd/spawni.c
> @@ -60,12 +60,12 @@ __spawni (pid_t *pid, const char *file,
>       that remains visible after an exec is registration with the proc
>       server, and the inheritance of various values and ports.  All those
>       inherited values and ports are what get collected up and passed in the
> -     file_exec RPC by an exec call.  So we do the proc server registration
> +     file_exec_file_name RPC by an exec call.  So we do the proc server 
> registration
>       here, following the model of fork (see fork.c).  We then collect up
>       the inherited values and ports from this (parent) process following
>       the model of exec (see hurd/hurdexec.c), modify or replace each value
>       that fork would (plus the specific changes demanded by ATTRP and
> -     FILE_ACTIONS), and make the file_exec RPC on the requested executable
> +     FILE_ACTIONS), and make the file_exec_file_name RPC on the requested 
> executable
>       file with the child process's task port rather than our own.  This
>       should be indistinguishable from the fork + exec implementation,
>       except that all errors will be detected here (in the parent process)

These are too long.  You should reformat the paragraph.

> @@ -622,14 +620,28 @@ __spawni (pid_t *pid, const char *file,
>  
>      inline error_t exec (file_t file)
>        {
> -     return __file_exec (file, task,
> -                         (__sigismember (&_hurdsig_traced, SIGKILL)
> -                          ? EXEC_SIGTRAP : 0),
> -                         args, argslen, env, envlen,
> -                         dtable, MACH_MSG_TYPE_COPY_SEND, dtablesize,
> -                         ports, MACH_MSG_TYPE_COPY_SEND, _hurd_nports,
> -                         ints, INIT_INT_MAX,
> -                         NULL, 0, NULL, 0);
> +     error_t err = __file_exec_file_name (file, task,
> +                                          (__sigismember (&_hurdsig_traced, 
> SIGKILL)
> +                                           ? EXEC_SIGTRAP : 0),
> +                                          filename,
> +                                          args, argslen, env, envlen,
> +                                          dtable, MACH_MSG_TYPE_COPY_SEND, 
> dtablesize,
> +                                          ports, MACH_MSG_TYPE_COPY_SEND, 
> _hurd_nports,
> +                                          ints, INIT_INT_MAX,
> +                                          NULL, 0, NULL, 0);

A bunch of lines should be broken here, but the first one is particularly
tricky.  Do something like this instead:

        error_t err = __file_exec_file_name
          (file, task,
           __sigismember (&_hurdsig_traced, SIGKILL) ? EXEC_SIGTRAP : 0,
           filename,
           args, argslen, env, envlen,
           dtable, MACH_MSG_TYPE_COPY_SEND, dtablesize,
           ports, MACH_MSG_TYPE_COPY_SEND, _hurd_nports,
           ints, INIT_INT_MAX,
           NULL, 0, NULL, 0);

(I removed the parenthesis surrounding __sigismember() because they're only
there for formatting.)

> +     /* Fallback for backwards compatibility.  This can just be removed
> +        when __file_exec goes away.  */
> +     if (err)
> +       return __file_exec (file, task,
> +                           (__sigismember (&_hurdsig_traced, SIGKILL)
> +                           ? EXEC_SIGTRAP : 0),
> +                           args, argslen, env, envlen,
> +                           dtable, MACH_MSG_TYPE_COPY_SEND, dtablesize,
> +                           ports, MACH_MSG_TYPE_COPY_SEND, _hurd_nports,
> +                           ints, INIT_INT_MAX,
> +                           NULL, 0, NULL, 0);
> +
> +     return err;
>        }
>  

Check for MIG_BAD_ID instead.

Regards,
  Fredrik