Re: [PATCH 2/3] Add the code for starting up the mountee

[Sergiu Ivanov]

Hello,

On Fri, Aug 07, 2009 at 07:58:34PM +0200, olafBuddenhagen@gmx.net wrote:
> On Mon, Aug 03, 2009 at 08:20:19PM +0300, Sergiu Ivanov wrote:
> > On Sat, Jul 18, 2009 at 06:33:11AM +0200, olafBuddenhagen@gmx.net wrote:
> > > On Thu, Jul 16, 2009 at 01:04:06PM +0300, Sergiu Ivanov wrote:
> 
> > > And I still don't like "np".
> > 
> > I looked through unionfs again and I can confirm that it uses ``np''
> > for ``node pointer'' everywhere.  Should I break the convention?  I do
> > agree that this isn't a very intuitive name, but I'm not sure what to
> > choose: a better name or consistency.
> 
> As I said before, I don't think this is a case where consistency is
> actually meaningful. It's not like "np" refers to the very same thing
> everywhere, and using a different name here would make the association
> harder. Rather, "np" is a very generic name, which refers to something
> different in every context -- the only thing in common being that it's
> always some node pointer. (Which is silly IMHO -- a variable name should
> carry the meaning of the variable, not it's type...)

I see.  The idea behind such notation is, probably, that in the
context of libnetfs stubs you actually work with ``node pointers'';
it's hard to think of something more descriptive than a ``node
pointer'' variable name in this case :-)
 
> But ultimately it's your decision -- it's not like using "np" here makes
> it any worse than the existing code... :-)

Great :-)
 
> > Note that the documentation for mach_port_deallocate says it's okay to
> > deallocate dead names (and I'd say MACH_PORT_NULL is not much worse
> > than a dead name).
> 
> I don't see how one follows from the other...

I was referring to the fact that fsys_getroot, in the majority of
cases, attempts to free dotdot (an in the majority of cases this is
the only operation done on dotdot).  So, if we give fsys_getroot a
null port for dotdot, it will eventually free a MACH_PORT_NULL.
 
> > > > +  /* The proxy node on which the mountee will be sitting must be able
> > > > +     to forward some of the RPCs coming from the mountee to the
> > > > +     underlying filesystem.  That is why we create this proxy node as
> > > > +     a clone of the root node: the mountee will feel as if there is no
> > > > +     unionfs under itself.  */
> > > > +  unionmount_proxy = netfs_make_node (netfs_root_node->nn);
> > > 
> > > It's confusing to call it "proxy", when we aren't doing any explicit
> > > proxying...
> > > 
> > > (In fact I don't think that any RPCs are actually forwarded this way at
> > > all?)
> > 
> > Of course, there is no *explicit* forwarding, but, as I remark in the
> > new comment to this line, most of the RPCs work out of the box,
> > because the netnode contained in the node to which I attach the
> > mountee is the same as in netfs_root_node.  For example, the
> > translator I test unionmount on io_stats its underlying node.  Since
> > the underlying node is actually a unionfs node, netfs_validate_stat is
> > invoked and this function processes the node in a usual way, fetching
> > valid stat information.
> 
> I do understand how it works. My point is that "proxy" is not the right
> term for this.

Ah, OK.  I changed this to mountee_node; it should be better.
 
> > > > +  err = start_mountee (unionmount_proxy, mountee_argz,
> > > > +                      mountee_argz_len, O_READ, &mountee_port);
> > > > +  if (err)
> > > > +    return err;
> > > > +
> > > > +  mountee_started = 1;
> > > > +
> > > > +  return err;
> > > > +}                              /* setup_unionmount */
> > > 
> > > "err" must always be 0 here, so it's probably clearer to just return 0.
> > > 
> > > Alternatively, you could make the "mounte_started" assignment
> > > conditional on !err, instead of returning early. This is often more
> > > elegant; the Hurd code uses this approach in many places.
> > 
> > I'll return 0, if you don't mind, because later patches introduce
> > actions in between mountee_started and return err, so rebasing would
> > imply introducing more if statements, which I would be happy to avoid.
> 
> This is not a good reason. If the situtation changes in later patches,
> you can change the approach there.

Yeah, sure :-) I'm just being lazy about changing ``0'' to ``err'' in
the later patches :-)
 
> > > (Same applies to startup_mountee(), BTW -- it *might* be more
> > > elegant to handle it this way... Though this is pretty
> > > case-specific; and I guess this is also a matter of taste to some
> > > extent at least.)
> > 
> > Generally, I prefer to avoid such ``elegant'' style.  I agree that it
> > looks aesthetically nice, but, when using this style, I often ran into
> > the necessity of building a structure of nested if statements, which
> > at a definite moment became too sophisticated to look nice.
> 
> It's not all-or-nothing. If you need to resort to nesting error
> conditions, then indeed things become ugly. But often enough, you get
> something like:
> 
>    if (!err)
>       do something;
>    if (!err)
>       do more stuff;
>    if (!err)
>       yet more;
> 
> This is very simple and obvious.

I've seen many an instance of such style in the Hurd code and I got to
like it in ~80% of cases.  The remainder ~20% are the ones with nested
if statements.  I'm not yet used to keeping to such style myself, but,
since I like it, I'll try to catch up with it.
 
> > I admit that I could have applied a little bit more effort and split
> > the tree-like if statements, but it's a hard task for me to understand
> > why I should apply more effort to maintain an ``elegant''
> > construction, if an absolutely equivalent though less elegant
> > structure keeps me out of trouble :-)
> 
> An elegant construction is one that is easy to maintain. What exactly
> falls in this catergory, is certainly a matter of taste; but the
> previous statement should always be true.

Hm, I'm used to hearing that an ``elegant approach'' is the one that
brings pleasure to your eyes :-) I've always liked the
efficiency-biased definition better :-)
 
> If there is a discrepancy between what you consider "aesthetically
> pleasing", and what you consider maintainable, you need to fix your
> sense of aesthetics ;-)

That's true :-) I sometimes feel like telling this to people trying to
write some perversely ``pleasant-looking'' code.

Regards,
scolobb