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

[olafBuddenhagen]

Hi,

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. Further explanation about
how this happens, would normally go into the body of the commit message
-- but as we know, GNU changelog policy is stuck in the 1980s and
forbids this... :-(

The fact that you submitted various variants of this patch with
different titles, is very confusing -- I actually failed to review some
versions, because I got quite mixed up as to which are newer variants of
this patch, and which are different things...

> * mount.c (mountee_node): New variable.
> (mountee_root): Likewise.
> (mountee_started): Likewise.
> (start_mountee): New function (based on node_set_translator
> in nsmux).
> (setup_unionmount): New function.
> * (mountee_root): New variable.

I think you forgot the file name here...

> (mountee_started): Likewise.
> (start_mountee): New function.
> (setup_unionmount): New function.
> * netfs.c (netfs_validate_stat): Start the mountee at the
> first invocation.
> ---
>  mount.c |  145 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  mount.h |   21 +++++++++
>  netfs.c |    7 +++
>  3 files changed, 173 insertions(+), 0 deletions(-)
> 
> diff --git a/mount.c b/mount.c
> index 7bc1fb8..45889f8 100644
> --- a/mount.c
> +++ b/mount.c
> @@ -22,8 +22,153 @@
>  
>  #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 * mountee_node;
> +
> +mach_port_t mountee_root;
> +
> +int mountee_started = 0;
> +
> +/* Starts the mountee (given by `argz` and `argz_len`), attaches it to
> +   the node `np` and opens a port `port` to the mountee with
> +   `flags`.  */
> +error_t
> +start_mountee (node_t * np, char * argz, size_t argz_len, int flags,
> +            mach_port_t * port)
> +{
> +  error_t err;
> +  mach_port_t underlying_port;
> +
> +  mach_port_t mountee_control;
> +
> +  /* Identity information about the unionfs process (for
> +     fsys_getroot).  */
> +  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 the unionfs process.  */
> +  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 the unionfs 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 protid which will contain the port to the node on which the
> +       mountee will be sitting.  */
> +    struct protid * newpi;
> +
> +    struct iouser * unionfs_user;
> +
> +    /* Initialize `unionfs_user` with the effective UIDs and GIDs of
> +       the unionfs process.  */
> +    err = iohelp_create_complex_iouser (&unionfs_user, uids, nuids, gids, 
> ngids);
> +    if (err)
> +      return err;
> +
> +    /* Create a port to `np`.  */

This comment is really why I dislike "np" so much...

If you insist on using a meaningless variable name, please at least let
the comment say what this is actually about :-)

> +    newpi = netfs_make_protid
> +      (netfs_make_peropen (np, flags, NULL), unionfs_user);
> +    if (!newpi)
> +      {
> +     iohelp_free_iouser (unionfs_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 */
> +
> +  /* 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, &mountee_control);
> +  if (err)
> +    return err;
> +
> +  /* Attach the mountee to the port opened in the previous call.  */
> +  err = file_set_translator (underlying_port, 0, FS_TRANS_SET, 0, argz,
> +                          argz_len, mountee_control, 
> MACH_MSG_TYPE_COPY_SEND);
> +  port_dealloc (underlying_port);
> +  if (err)
> +    return err;
> +
> +  /* Obtain the port to the root of the newly-set translator.  */
> +  err = fsys_getroot (mountee_control, MACH_PORT_NULL, 
> MACH_MSG_TYPE_COPY_SEND,
> +                   uids, nuids, gids, ngids, flags, &retry_port,
> +                   retry_name, port);
> +  if (err)
> +    return err;
> +
> +  return 0;

The last four lines are silly... Just return err and be done with it.

I think this actually hints that the other style of error handling I
suggested would indeed be more appropriate here... :-)

> +}                            /* 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)

The comment is wrong I think: You do not actually register the mountee
yet; this is done only with the next patch...

> +{
> +  error_t err = 0;
> +
> +  /* 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...

> +  mountee_node = netfs_make_node (netfs_root_node->nn);
> +  if (!mountee_node)
> +    return ENOMEM;
> +
> +  /* 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?...

> +  err = start_mountee (mountee_node, mountee_argz, mountee_argz_len,
> +                    O_READ, &mountee_root);
> +  if (err)
> +    return err;
> +
> +  mountee_started = 1;
> +
> +  return 0;
> +}                            /* setup_unionmount */
> +
> diff --git a/mount.h b/mount.h
> index a7dd933..53679ad 100644
> --- a/mount.h
> +++ b/mount.h
> @@ -23,10 +23,31 @@
>  #ifndef INCLUDED_MOUNT_H
>  #define INCLUDED_MOUNT_H
>  
> +#include <error.h>
>  #include <unistd.h>
> +#include <hurd/hurd_types.h>
> +
> +#include "node.h"
>  
>  /* The command line for starting the mountee.  */
>  extern char * mountee_argz;
>  extern size_t mountee_argz_len;
>  
> +extern mach_port_t mountee_root;
> +
> +extern int mountee_started;
> +
> +/* Starts the mountee (given by `argz` and `argz_len`), attaches it to
> +   the node `np` and opens a port `port` to the mountee with
> +   `flags`.  */
> +error_t
> +start_mountee (node_t * np, char * argz, size_t argz_len, int flags,
> +            mach_port_t * port);
> +
> +/* 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);
> +
>  #endif /* not INCLUDED_MOUNT_H */
> diff --git a/netfs.c b/netfs.c
> index 8d2a4ec..f8d5e3c 100644
> --- a/netfs.c
> +++ b/netfs.c
> @@ -199,6 +199,13 @@ netfs_validate_stat (struct node *np, struct iouser 
> *cred)
>      }
>    else 
>      {
> +      if (!mountee_started)
> +     {
> +       err = setup_unionmount ();
> +       if (err)
> +         error (EXIT_FAILURE, err, "failed to setup the mountee");

When used as verb, "set up" remain seperate words.

> +     }
> +
>        _get_node_size (np, &np->nn_stat.st_size); 
>      }

-antrik-