Hello,
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:
> > +/* The node the mountee is sitting on. */
> > +node_t * unionmount_proxy;
> > +
> > +mach_port_t mountee_port;
>
> This is the control port of the mountee I assume? Perhaps it would be
> useful to mention it, either in a comment, or even in the variable name
> itself. (i.e. "mountee_control" or "mountee_ctrl")
Nope, it's the port to the root of the mountee. I realize now that
this name is indeed not suggestive, so I changed it to mountee_root,
as you pointed out in one of your reviews. Also, I added a comment to
avoid ambiguity.
> > +/* Starts the mountee (given by `argz` and `argz_len`), sets it on
> > + node `np` and opens a port `port` to with `flags`. `port` is not
> > + modified when an error occurs. */
> > +error_t
> > +start_mountee (node_t * np, char * argz, size_t argz_len, int flags,
> > + mach_port_t * port)
>
> [...]
> 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.
> > +{
> > + error_t err;
> > + mach_port_t underlying_port;
> > +
> > + /* The intermediate container for the port to the root of the
> > + mountee. */
> > + mach_port_t res_port;
>
> The name is still not very descriptive. Something like "mountee_root"
> would be. You don't need the comment at all in this case, as the name
> says it all...
>
> As I said in the other mail, I'm still not convinced that it's actually
> useful to use a temporary variable here at all... But again, it's not
> really important -- I wouldn't complain if the patch was perfect
> otherwise :-)
I thought of it a little more and decided to drop the temporary
variable. I already have the global variable mountee_root and I would
like to avoid choosing a different suitable name for a
not-really-useful local variable.
> > +
> > + /* An unauthenticated port to the root of the translator, which
> > + plays the role of the directory containing the underlying node of
> > + the mountee. This one is used in fsys_getroot as the dotdot
> > + parameter, so it is not really important what we put here because
> > + the dotdot parameter is used mostly with symlinks. */
> > + mach_port_t unauth_dir;
>
> Actually, dotdot can be used by all kinds of relative lookups AIUI; it
> has nothing to do with symlinks...
My conclusion about dotdot and symlinks derives from what I can see in
netfs_S_fsys_get_root (hurd/libnetfs/fsys-getroot.c:110): dotdot
appears to be used in a meaningful way when the translator is a
symlink. In all other cases dotdot is either deallocated or fed to
fshelp_fetch_root, which ultimately calls fsys_getroot on another
translator.
> I don't see though why unionfs would ever do relative lookups on any of
> the merged filesystems -- so your conclusion that this doesn't actually
> play a role is probably correct; only your explanation is wrong :-)
Changing the explanation is much easier than changing the code ;-)
> However, I don't think it's helpful to misuse the unionfs root node for
> that. I tend to think that it would be best just to create a dummy port.
> (No RPCs should ever be invoked on it anyways, right?)
>
> Or maybe even just pass MACH_PORT_NULL, if that's possible?...
Frankly, I was thought of passing MACH_PORT_NULL, but some obscure
thoughts of impropriety hindered me. I tried passing MACH_PORT_NULL
as dotdot and it works pretty well. 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).
> > +/* Sets up a proxy node, sets the translator on it, and registers the
> > + filesystem published by the translator in the list of merged
> > + filesystems. */
> > +error_t
> > +setup_unionmount (void)
> > +{
> > + error_t err = 0;
> > +
> > + /* 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.
> Indeed, the comment is fundamentally wrong: the whole point of using the
> unionfs root node here is so that the mountee *does* see the unionfs
> under it -- while in the transparent case, we will (probably) use the
> underlying node of unionfs instead, so that the mountee really doesn't
> see unionfs.
Yeah, sure. I wanted to mean something like proxying, but apparently
ran into formulation problems.
> > + if (!unionmount_proxy)
> > + return ENOMEM;
> > +
> > + /* Set the mountee on the proxy node.
> > + Note that the O_READ flag does not actually limit access to the
> > + mountee's filesystem considerably. Whenever a client looks up a
> > + node which is not a directory, unionfs will give off a port to
> > + the node itself, withouth proxying it. Proxying happens only for
> > + directory nodes. */
> > + 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.
> (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. 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 :-)
Also, I believe gcc optimizes all these statements anyways, so both
styles most probably end up in the same machine code.
Regards,
scolobb
