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

[olafBuddenhagen]

Hi,

On Mon, Aug 03, 2009 at 11:37:50PM +0300, Sergiu Ivanov wrote:
> 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:

> > 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 don't know about fshelp_start_translator_long(); but boot was actually
the example I know. It uses notify_S.h and notifyServer.o, which are
generated with MiG from notify.defs.

> > > +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?

Because it's not necessary to implement the functionality of the patch.
It's refactoring, which is useful and important, but better be kept in
extra patches, so it doesn't obscure the functional changes in the main
patch.

> > > +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 :-(

I'm talkin about the "/* _mountee_listen_thread */" comment at the end
of the function. These comments might be useful for long functions,
where the beginning is not visible while looking at the end -- but for a
function that only has a few lines, it's pointless. It's just an
unnecessary maintenance burden -- it seems that you actually forgot to
update it at some point, as the comment doesn't match the function name.

-antrik-