Re: [PATCH 2/3] Start the mountee in a lazy fashion.

[olafBuddenhagen]

Hi,

On Mon, Aug 17, 2009 at 07:15:09PM +0300, Sergiu Ivanov wrote:
> On Sun, Aug 16, 2009 at 07:42:30PM +0200, olafBuddenhagen@gmx.net wrote:
> > On Mon, Aug 03, 2009 at 08:41:55PM +0300, Sergiu Ivanov wrote:

> > > Subject: [PATCH 2/3] Start the mountee in a lazy fashion.
> > 
> > This title line is not very helpful. The important point is that
> > this patch adds the code for starting the mountee.
> 
> AIUI, this title line does say that some code for starting the mountee
> is added.  Or do I understand something wrong?

The fact that you mention the specific detail "in a lazy fashion",
creates the impression that this is the major point -- it sounds like
the purpose of this patch is changing the code to use "lazy fashion"
instead of some other approach used before... It's not at all clear that
this patch adds the mountee starting code for the first time.

> I changed the title of this patch because the very first one -- ``Add
> the code for starting up the mountee'' -- is too verbose (almost every
> patch *adds* some code).

I don't think it is too verbose.

In either case, I don't quite understand that you try to make it less
verbose by adding more detail?... :-)

"Add mountee startup" or "implement mountee startup" would be perfectly
fine -- saying the same thing in less words. Or even just "start
mountee", if you think the "add" is superfluous.

> I won't submit patch with corrections you mention in this E-mail right
> away, because the corrections are mainly about changing some comments
> or strings and I think it will be harder for you to review the changes
> if I post the whole patch again.

Well, I can't really give a final ACK without seeing the whole patch in
its final form...

> > > +  /* The mountee will be sitting on this node.  This node is based on
> > > +     the netnode of the root node, so most RPCs on this node can be
> > > +     automatically carried out correctly.  */
> > 
> > I guess it would be helpful to explicitely mention the word "clone"
> > somewhere. Also, the comment should explain *why* it is necessary to
> > clone, instead of just using the original node...
> 
> Changed to: ``The mountee will be sitting on this node.  This node is
> based on the netnode of the root node (it is essentially a clone of
> the root node), so most RPCs on this node can be automatically carried
> out correctly.  Note the we cannot set the mountee on the root node
> directly, because in this case the mountee's filesystem will obscure
> the filesystem published by unionfs.''

"most RPCs ont this node can be automatically carried out correctly" is
way too vague... It's not ever clear what "correct" means in here, no
what RPCs you mean.

I think you should say that the mountee is set on a (clone of) the
unionfs root node, so that unionfs appears as the parent translator of
the mountee. AIUI that's the idea behind it, right?

> > > +  /* Set the mountee on the new 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.  */
> > 
> > Why are you passing O_READ, anyways?...
> 
> The flags which I pass to start_mountee are used in opening the port
> to the root node of the mountee.  (I'm sure you've noticed this; I'm
> just re-stating it to avoid ambiguities).  Inside unionfs, this port
> is used for lookups *only*, so O_READ should be sufficient for any
> internal unionfs needs.  Ports to files themselves are not proxied by
> unionfs (as the comment reads), so the flags passed here don't
> influence that case.

Hm, but wouldn't unionfs still need write permissions to the directories
for adding new entries, when not in readonly mode?...

Also, if it's *always* O_READ, why do you pass it as a function
parameter at all?

-antrik-