bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Sergiu Ivanov
Subject: Re: [PATCH 2/4] Go away when the mountee has been shut down.
Date: Mon, 17 Aug 2009 21:56:07 +0300
User-agent: Mutt/1.5.16 (2007-06-09)

Hello,

On Sun, Aug 16, 2009 at 09:55:26PM +0200, olafBuddenhagen@gmx.net wrote:
> 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:
> 
> > > 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.

Yes, you are right, boot uses notify.defs.  When looking into the code
now that I know a little bit more, I can notice more details.
However, note that the notify_server demuxer is actually invoked from
the request_server, which is used as the handler function in the call
to mach_msg_server.  This means that, canonically, I would need to
employ the notify interface, import the notify_server demuxer and
invoke it.  Instead, I do the required simple handling myself.  I call
this handling ``simple'', because it actually requires one if
statement, versus the more complicated way when MiG is implied.

I'd rather stay with the current implementation, because it indeed is
simpler and I cannot find any circumstances under which it would work
worse that the more generic implementation via the notify interface.
However, should you insist on keeping to the standard way, I'll (try
to) implement the functionality using 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.

I can see your point, but I fail to figure out how would this
modification to the patch obscure the functional changes it
introduces?  After all, the whole refactoring part consists of
removing one variable and modifying a single condition (IIRC).  Do I
understand something wrong?

Regards,
scolobb




reply via email to

[Prev in Thread] Current Thread [Next in Thread]