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 <unlimitedscol...@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-
