Hello, On Fri, Aug 07, 2009 at 07:58:34PM +0200, olafbuddenha...@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, 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...)
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