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

[olafBuddenhagen]

Hi,

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...)

But ultimately it's your decision -- it's not like using "np" here makes
it any worse than the existing code... :-)

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

> > > +  /* 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.

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

But I don't really care either way...

> > (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 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.

If there is a discrepancy between what you consider "aesthetically
pleasing", and what you consider maintainable, you need to fix your
sense of aesthetics ;-)

> Also, I believe gcc optimizes all these statements anyways, so both
> styles most probably end up in the same machine code.

Sure.

-antrik-