Hi,
On Mon, Aug 03, 2009 at 08:20:19PM +0300, Sergiu Ivanov wrote:
> On Sat, Jul 18, 2009 at 06:33:11AM +0200, olafbuddenha...@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-
