Re: [PATCH 2/4] Go away when the mountee has been shut down.

[olafBuddenhagen]

Hi,

On Fri, Jul 17, 2009 at 01:57:33PM +0300, Sergiu Ivanov wrote:

> >From ba1a38092c3b79c5c4668c159a7a1640c6d94c51 Mon Sep 17 00:00:00 2001
> From: Sergiu Ivanov <unlimitedscolobb@gmail.com>
> Date: Tue, 14 Jul 2009 19:41:41 +0000
> Subject: [PATCH 2/4] Go away when the mountee has been shut down.
> 
> * mount.c (mountee_control): New variable.
> (mountee_listen_port): Likewise.
> (start_mountee): Store the control port of the mountee in
> mountee_control.
> (mountee_server): New function.
> (_mountee_listen_thread_proc): Likewise.
> (setup_unionmount): Request to be notified when the mountee goes
> away.  Detach a separate thread to wait for the notification.

Hm... While this keeps the code surprisingly simple, it is a rather
unusual approach. Have you seen any example of handling it like this in
existing Hurd code? The approach I've seen so far is letting MiG create
a notify_server, and including it in the main RPC multiplexer...

> * mount.h (mountee_control): New variable.
> ---
>  mount.c |   64 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  mount.h |    1 +
>  2 files changed, 65 insertions(+), 0 deletions(-)
> 
> diff --git a/mount.c b/mount.c
> index 4d12cd6..26cbd9f 100644
> --- a/mount.c
> +++ b/mount.c
> @@ -24,6 +24,7 @@
>  
>  #include <hurd/fsys.h>
>  #include <fcntl.h>
> +#include <cthreads.h>
>  
>  #include "mount.h"
>  #include "lib.h"
> @@ -37,6 +38,7 @@ size_t mountee_argz_len;
>  node_t * unionmount_proxy;
>  
>  mach_port_t mountee_port;
> +mach_port_t mountee_control = MACH_PORT_NULL;

There is no point in initializing this explicitely, unless you actually
check for it being set somewhere (e.g. have an "assert(mountee_control
!= MACH_PORT_NULL)" somewhere), or otherwise make some call that could
happen without this being set...

OTOH, I think we could use this to indicate that the mountee has been
started, instead of the the extra "mountee_started" flag -- simple and
elegant. Perhaps you could create a followup patch doing this?

>  
>  int mountee_started = 0;
>  
> @@ -44,6 +46,10 @@ int mountee_started = 0;
>     operates (transparent/non-transparent).  */
>  int transparent_mount = 1;
>  
> +/* The port for receiving the notification about the shutdown of the
> +   mountee.  */
> +mach_port_t mountee_listen_port;

I think "notify" instead of "listen" in the variable name would be more
explicit...

> +
>  /* Starts the mountee (given by `argz` and `argz_len`), sets it on
>     node `np` and opens a port `port` to with `flags`.  `port` is not
>     modified when an error occurs.  */
> @@ -176,10 +182,35 @@ start_mountee (node_t * np, char * argz, size_t 
> argz_len, int flags,
>      }
>  
>    *port = res_port;
> +  mountee_control = control;
>  
>    return 0;
>  }                            /* start_mountee */
>  
> +/* Listens to the MACH_NOTIFY_NO_SENDERS notification for the port to
> +   the root of the mountee.  */

This comment seems wrong: as far as I can see, you actually (correctly)
listen on the control port, not the root node port...

> +error_t
> +mountee_server (mach_msg_header_t * inp, mach_msg_header_t * outp)
> +{
> +  if (inp->msgh_id == MACH_NOTIFY_DEAD_NAME)
> +    {
> +      /* Terminate operations conducted by unionfs and shut down.  */
> +      netfs_shutdown (FSYS_GOAWAY_FORCE);
> +      exit (0);
> +    }
> +
> +  return 1;

Why 1?

> +}                            /* mountee_server */
> +
> +/* The main proc of thread for listening for the MACH_NOTIFY_DEAD_NAME
> +   notification on the control port of the mountee.  */
> +static void
> +_mountee_listen_thread_proc (any_t * arg)
> +{
> +  while (1)
> +    mach_msg_server (mountee_server, 0, mountee_listen_port);
> +}                            /* _mountee_listen_thread */

The comment here seems pointless, when the function started only a few
lines above...

Also, it's wrong :-)

> +
>  /* Sets up a proxy node, sets the translator on it, and registers the
>     filesystem published by the translator in the list of merged
>     filesystems.  */
> @@ -214,6 +245,39 @@ setup_unionmount (void)
>  
>    mountee_started = 1;
>  
> +  /* The previously registered send-once right (used to hold the value
> +     returned from mach_port_request_notification) */
> +  mach_port_t prev;
> +
> +  /* Setup the port for receiving notifications.  */
> +  err = mach_port_allocate (mach_task_self (), MACH_PORT_RIGHT_RECEIVE,
> +                   &mountee_listen_port);
> +  if (err)
> +    return err;
> +  err = mach_port_insert_right (mach_task_self (), mountee_listen_port,
> +                             mountee_listen_port, MACH_MSG_TYPE_MAKE_SEND);
> +  if (err)
> +    {
> +      mach_port_deallocate (mach_task_self (), mountee_listen_port);
> +      return err;
> +    }
> +
> +  /* Request to be notified when the mountee goes away and
> +     `mountee_control` becomes a dead name.  */
> +  err = mach_port_request_notification (mach_task_self (), mountee_control,
> +                                     MACH_NOTIFY_DEAD_NAME, 1, 
> mountee_listen_port,
> +                                     MACH_MSG_TYPE_MAKE_SEND_ONCE, &prev);
> +  if (prev != MACH_PORT_NULL)
> +    mach_port_deallocate (mach_task_self(), prev);

I don't think this should ever happen -- who could have registered a
previous notification?...

In other words, make it an assert().

-antrik-