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

[olafBuddenhagen]

Hi,

On Thu, Jul 16, 2009 at 01:04:06PM +0300, Sergiu Ivanov wrote:

> diff --git a/mount.c b/mount.c
> index 7bc1fb8..7e134d6 100644
> --- a/mount.c
> +++ b/mount.c
> @@ -22,8 +22,200 @@
>  
>  #define _GNU_SOURCE
>  
> +#include <hurd/fsys.h>
> +#include <fcntl.h>
> +
>  #include "mount.h"
> +#include "lib.h"
>  
>  /* The command line for starting the mountee.  */
>  char * mountee_argz;
>  size_t mountee_argz_len;
> +
> +/* 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")

Not really important though, just a random idea :-)

> +
> +int mountee_started = 0;
> +
> +/* 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)

As I already said in another place, I think it's clearer to say that the
mountee is *attached* to the node -- "set" is quite ambiguous.

Also, the sentence is broken.

And I still don't like "np".

> +{
> +  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 :-)

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

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

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

> +
> +  /* The control port of the mountee.  */
> +  mach_port_t control;

Maybe name it mountee_control (or mountee_ctrl), and drop the
comment?...

> +
> +  /* The user who has no priveleges at all.  */
> +  struct iouser * nobody;
> +
> +  /* A protid which will be created from the supplied node.  */
> +  struct protid * newpi;

This comment doesn't really tell anything.

BTW, it seems to me that the use of newpi inside open_port() is totally
unrelated to the use outside of it? In that case, it probably would be
better to use a distinct variable...

> +
> +  /* Identity information about the current process (for
> +     fsys_getroot).  */

Perhaps it's clearer to explicitely say that its the *unionfs* process,
not just "current"?

> +  uid_t * uids;
> +  size_t nuids;
> +
> +  gid_t * gids;
> +  size_t ngids;
> +
> +  /* The retry information returned by fsys_getroot.  */
> +  string_t retry_name;
> +  mach_port_t retry_port;
> +
> +  /* Fetch the effective UIDs of this process.  */

Same here.

> +  nuids = geteuids (0, 0);
> +  if (nuids < 0)
> +    return EPERM;
> +  uids = alloca (nuids * sizeof (uid_t));
> +
> +  nuids = geteuids (nuids, uids);
> +  assert (nuids > 0);
> +
> +  /* Fetch the effective GIDs of this process.  */
> +  ngids = getgroups (0, 0);
> +  if (ngids < 0)
> +    return EPERM;
> +  gids = alloca (ngids * sizeof (gid_t));
> +
> +  ngids = getgroups (ngids, gids);
> +  assert (ngids > 0);
> +
> +  /* Opens the port on which to set the mountee.  */
> +  error_t open_port (int flags, mach_port_t * underlying,
> +                  mach_msg_type_name_t * underlying_type, task_t task,
> +                  void *cookie)
> +  {
> +    err = 0;
> +
> +    /* The user which has created this process.  */
> +    struct iouser * user;

"User" is confusing here IMHO: an iouser is not quite the same thing as
a user in the normal UNIX sense. What we actually have here is an iouser
describing the authority of the unionfs process.

Note that while it will usually contain only the ID of the user who
actually started unionfs, this needn't always be the case.

BTW, if you really think that the meaning of this variable is not clear
enough by itself, then perhaps it might be better to use a more verbose
variable name?... (e.g. unionfs_user)

> +
> +    /* Create an iouser instance basing on the obtained authority
> +       information.  */
> +    err = iohelp_create_complex_iouser (&user, uids, nuids, gids, ngids);

Another comment that doesn't really tell me anything...

> +    if (err)
> +      return err;
> +
> +    /* Create a port to `np`.  */
> +    newpi = netfs_make_protid
> +      (netfs_make_peropen (np, flags, NULL), user);
> +    if (!newpi)
> +      {
> +     iohelp_free_iouser (user);
> +     return errno;
> +      }
> +
> +    *underlying = underlying_port = ports_get_send_right (newpi);
> +    *underlying_type = MACH_MSG_TYPE_COPY_SEND;
> +
> +    ports_port_deref (newpi);
> +
> +    return err;
> +  }                          /*open_port */
> +
> +  /* Create a completely unprivileged user.  */
> +  err = iohelp_create_iouser(&nobody, NULL, NULL);
> +  if(err)
> +    return err;
> +
> +  /* Create a port to `np` for the unprivileged user.  */
> +  newpi = netfs_make_protid
> +    (netfs_make_peropen (np, flags, NULL), nobody);
> +  if (!newpi)
> +    {
> +      iohelp_free_iouser (nobody);
> +      err = errno;
> +      return err;
> +    }
> +
> +  unauth_dir = ports_get_send_right (newpi);
> +  ports_port_deref (newpi);
> +
> +  /* Start the translator.  The value 60000 for the timeout is the one
> +     found in settrans.  */
> +  err = fshelp_start_translator (open_port, NULL, argz, argz, argz_len,
> +                              60000, &control);
> +  if (err)
> +    {
> +      iohelp_free_iouser (nobody);
> +      return err;
> +    }
> +
> +  /* Attempt to attach the mountee to the port opened in the previous
> +     call.  */

Not helpful to say "attempt" IMHO -- we actually expect this to succeed,
and bail out otherwise...

> +  err = file_set_translator (underlying_port, 0, FS_TRANS_SET, 0, argz,
> +                          argz_len, control, MACH_MSG_TYPE_COPY_SEND);
> +  port_dealloc (underlying_port);
> +  if (err)
> +    {
> +      iohelp_free_iouser (nobody);
> +      return err;
> +    }
> +
> +  /* Obtain the port to the root of the newly-set translator.  */
> +  err = fsys_getroot (control, unauth_dir, MACH_MSG_TYPE_COPY_SEND,
> +                   uids, nuids, gids, ngids, flags, &retry_port,
> +                   retry_name, &res_port);
> +  if (err)
> +    {
> +      iohelp_free_iouser (nobody);
> +      return err;
> +    }
> +
> +  *port = res_port;
> +
> +  return 0;
> +}                            /* start_mountee */
> +
> +/* 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?)

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.

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

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

-antrik-