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

[Sergiu Ivanov]

Hello,

On Wed, Jul 29, 2009 at 10:47:53AM +0200, olafBuddenhagen@gmx.net wrote:
> 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...

My implementation is mainly based on boot and
fshelp_start_translator_long.  boot uses mach_msg_server, while
fshelp_start_translator_long uses some special mechanism, which I
didn't take the time to understand completely.  Neither uses MiG,
AFAICT.

I see the reason in the fact that reply servers are based on MiG
subsystems, and I cannot see a subsystem which would allow to listen
to MACH_NOTIFY_* notifications.  I'm not even sure that waiting on the
mountee to shut down can be achieved in a way different from what I
implemented, but this is due to lack of knowledge.
 
> > * 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?

Well, it's easy to create the patch, but I wonder why wouldn't I
modify this patch directly?
 
> > +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?

As far as I could deduce from hurd/boot/boot.c , the function supplied
to mach_msg_server should return 1 on success.  I guess it's very
similar to the case of a classical demuxer function which joins
several server functions with logical OR (``||''), but I'm not sure.
 
> > +}                          /* 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 :-)

I'm not sure I can correctly understand what you are talking about :-(

Regards,
scolobb