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

[olafBuddenhagen]

Hi,

On Thu, Jun 11, 2009 at 09:19:21PM +0300, Sergiu Ivanov wrote:

> * netfs.c: Update copyright years.
> (netfs_S_dir_lookup): Add the code for starting up the mountee
> at the first invocation.

Just "Start up mountee on first invocation" is probably better.

> (netfs_get_dirents): Likewise.
> 
> * unionmount.h: (unionmount_proxy): Declare this variable.

AFAIK "New variable" is the standard text.

> (mountee_port): Likewise.
> (mountee_started): Likewise.
> (unionmount_start_mountee): Declare this function.

Same here. ("New function")

> (unionmount_setup): Likewise.
> 
> * unionmount.c: (unionmount_proxy): Define this variable.
> (mountee_port): Likewise.
> (mountee_started): Likewise.
> (unionmount_start_mountee): Borrow the code of this function
> from node_set_translator in nsmux and adapt its variable names.

"New function (based on node_set_translator in nsmux)" or something like
that I'd say.

> (unionmount_setup): Define this function.
> ---
>  netfs.c      |   38 +++++++++++++-
>  unionmount.c |  168 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  unionmount.h |   26 +++++++++
>  3 files changed, 231 insertions(+), 1 deletions(-)
> 
> diff --git a/netfs.c b/netfs.c
> index 89d1bf6..fce3366 100644
> --- a/netfs.c
> +++ b/netfs.c
> @@ -1,7 +1,11 @@
>  /* Hurd unionfs
> -   Copyright (C) 2001, 2002, 2003, 2005 Free Software Foundation, Inc.
> +   Copyright (C) 2001, 2002, 2003, 2005, 2009 Free Software
> +   Foundation, Inc.
> +
>     Written by Moritz Schulte <moritz@duesseldorf.ccc.de>.
>  
> +   Adapted to unionmount by Sergiu Ivanov <unlimitedscolobb@gmail.com>

Adapted *for* unionmount is more correct I think.

> +
>     This program is free software; you can redistribute it and/or
>     modify it under the terms of the GNU General Public License as
>     published by the Free Software Foundation; either version 2 of the
> @@ -36,6 +40,7 @@
>  #include "lib.h"
>  #include "ncache.h"
>  #include "options.h"
> +#include "unionmount.h"
>  
>  /* Return an argz string describing the current options.  Fill *ARGZ
>     with a pointer to newly malloced storage holding the list and *LEN
> @@ -810,6 +815,21 @@ netfs_S_dir_lookup (struct protid *diruser,
>        goto gotit;
>      }
>  
> +  /*Start the mountee if it's not running yet. */
> +  if (!mountee_started)
> +    {
> +      error = unionmount_setup (diruser);
> +      if (error)
> +     {
> +     /*This is actually the end of initialization, so if something
> +       goes bad here we are rightful to die. And, of course,
> +       unionmount makes no sense if the mountee is not running. */
> +       errno = error;
> +       perror ("failed to setup the mountee");
> +       exit (EXIT_FAILURE);

Just use the error() function.

> +     }
> +    }
> +
>    dnp = diruser->po->np;
>  
>    mutex_lock (&dnp->lock);
> @@ -1112,6 +1132,22 @@ netfs_get_dirents (struct iouser *cred, struct node 
> *dir,
>       return 0;
>      }
>  
> +  /*Start the mountee if it's not running yet. */
> +  if (!mountee_started)
> +    {
> +      err = unionmount_setup
> +     (netfs_make_protid (netfs_make_peropen (dir, O_READ, NULL), cred));

I'm not sure about the exact convention, but I think the opening
parenthesis should go on the first line?...

Also, netfs_make_protid (and the following parenthesis) should probably
go on the first line too.

> +      if (err)
> +     {
> +     /*This is actually the end of initialization, so if something
> +       goes bad here we are rightful to die. And, of course,
> +       unionmount makes no sense if the mountee is not running. */
> +       errno = err;
> +       perror ("failed to setup the mountee");
> +       exit (EXIT_FAILURE);
> +     }
> +    }

Seems strange that almost the same code is needed twice; and indeed I
think it's wrong: how can netfs_get_dirents() ever be called before any
lookup has been done?...

> +
>    err = node_entries_get (dir, &dirent_list);
>  
>    if (! err)
> diff --git a/unionmount.c b/unionmount.c
> index e4aa043..0e3317d 100644
> --- a/unionmount.c
> +++ b/unionmount.c
> @@ -21,8 +21,176 @@
>  
>  #define _GNU_SOURCE 1
>  
> +#include <hurd/fsys.h>
> +#include <fcntl.h>
> +
> +#include "lib.h"
> +#include "ulfs.h"
>  #include "unionmount.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;
> +
> +/*The port to the mountee. */
> +mach_port_t mountee_port;

This comment seems rather useless -- it doesn't tell anything the
variable name itself doesn't tell...

> +
> +/*Shows whether the mountee has been started already. */
> +int mountee_started = 0;

Probably not necessary either...

> +
> +/*Starts the mountee (given by `argz` and `argz_len`), sets it on node
> +  `np` and opens a port `port` to with `flags`. */
> +error_t
> +unionmount_start_mountee (struct protid * diruser, node_t * np, char * argz,
> +                       size_t argz_len, int flags, mach_port_t * port)
> +{

"np" is not a very self-explaining variable name... Unless there is some
convention that makes it worthwhile to stick to it, better use a more
verbose name.

the unionmount_ prefix is not necessary I'd say -- we are not in a
library, so namespace pollution is not an issue. And it's obvious from
the context that this is unionmount-related.

> +  error_t err;
> +  mach_port_t p;

Again, I advocate a more verbose name...

> +
> +  /*An unauthenticated port to the directory containing `np` */
> +  mach_port_t unauth_dir;

Err... The mountee is set on an internal node, not attached to the real
filesystem. What is the "directory containing np" in this case?...

> +
> +  /*The control port for the active translator */
> +  mach_port_t active_control;

The "active" part seems redundant -- only active translators have
control ports...

> +
> +  /*A copy of the user information, supplied in `user` */
> +  struct iouser * user;
> +
> +  /*A protid for the supplied node */
> +  struct protid * newpi;

These variables are used only in open_node() I believe?

Always define variables as close to the code actually using them as
possible.

(I actually even tend to create extra code blocks only to be able to
define certain variables in a more local scope...)

> +
> +  /*Identity information about the current 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;
> +
> +  /*Try to get the number of effective UIDs */
> +  nuids = geteuids (0, 0);
> +  if (nuids < 0)
> +    return EPERM;
> +
> +  /*Allocate some memory for the UIDs on the stack */
> +  uids = alloca (nuids * sizeof (uid_t));

Hm, alloca() always seems a bit dirty... Though other parts of the Hurd
seem to use it as well, so I guess it's fine.

> +
> +  /*Fetch the UIDs themselves */
> +  nuids = geteuids (nuids, uids);
> +  if (nuids < 0)
> +    return EPERM;

This call shouldn't fail, as it succeeded just above. Use assert() to
check that it didn't happen nonetheless for some unexplicable reason...

> +
> +  /*Try to get the number of effective GIDs */
> +  ngids = getgroups (0, 0);
> +  if (ngids < 0)
> +    return EPERM;
> +
> +  /*Allocate some memory for the GIDs on the stack */
> +  gids = alloca (ngids * sizeof (gid_t));
> +
> +  /*Fetch the GIDs themselves */
> +  ngids = getgroups (ngids, gids);
> +  if (ngids < 0)
> +    return EPERM;
> +
> +  /*Opens the port on which to set the new translator */
> +  error_t
> +    open_port
> +    (int flags, mach_port_t * underlying,
> +     mach_msg_type_name_t * underlying_type, task_t task, void *cookie)

AFAIK open_port should not be indented, and the parameter list should
start on the same line.

> +  {
> +    err = 0;
> +
> +    /*Duplicate the supplied user */
> +    err = iohelp_dup_iouser (&user, diruser->user);

Not a very useful comment IMHO. You could try to explain *why* it
duplicates it -- or just leave it out alltogether.

> +    if (err)
> +      return err;
> +
> +    /*Create a protid for this node */
> +    newpi = netfs_make_protid
> +      (netfs_make_peropen (np, flags, diruser->po), user);

Same here.

Also, strange line wrapping again...

> +    if (!newpi)
> +      {
> +     iohelp_free_iouser (user);
> +     return errno;
> +      }
> +
> +    /*Obtain the resulting port right and set its type appropriately */
> +    *underlying = p = ports_get_send_right (newpi);
> +    *underlying_type = MACH_MSG_TYPE_COPY_SEND;

What is a "resulting port right"?

In the current form, the comment is not really useful either I believe.

> +
> +    /*Drop our reference to the port */
> +    ports_port_deref (newpi);
> +
> +    /*Return the result of operations (everything should be okay here) */
> +    return err;

Another pointless comment...

> +  }                          /*open_port */
> +
> +  /*Obtain the unauthenticated port to the directory */

Which directory?

> +  err = io_restrict_auth
> +    (diruser->po->np->nn->ulfs[0].port, &unauth_dir, 0, 0, 0, 0);
> +  if (err)
> +    return err;
> +
> +  /*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, &active_control);
> +  if (err)
> +    return err;
> +
> +  /*Attempt to set a translator on the port opened by the previous call */
> +  err = file_set_translator
> +    (p, 0, FS_TRANS_SET, 0, argz, argz_len,
> +     active_control, MACH_MSG_TYPE_COPY_SEND);

*Attach* the translator, not set it. "setting" a translator is what
settrans does.

> +  port_dealloc (p);
> +  if (err)
> +    return err;
> +
> +  /*Obtain the port to the top of the newly-set translator */

It's called "root" (or perhaps "root node"), not "top".

> +  err = fsys_getroot
> +    (active_control, unauth_dir, MACH_MSG_TYPE_COPY_SEND,
> +     uids, nuids, gids, ngids, flags, &retry_port, retry_name, &p);
> +  if (err)
> +    return err;
> +
> +  /*Return the port */
> +  *port = p;

Don't reuse the same variable name for something different -- it's very
confusing and error prone.

Also, why not just pass "port" to fsys_getroot() directly?

> +
> +  /*Everything is OK here */
> +  return 0;

Pointless comment...

> +}                            /* unionmount_start_trans */
> +
> +/*Sets up a proxy node, sets the translator on it, and registers the
> +  filesystem published by the translator in the list of merged
> +  filesystems. */
> +/*Diruser is required for some additional information. */

"Some additional information"?...

Either explain what it does, or don't comment it at all.

> +error_t
> +unionmount_setup (struct protid * diruser)
> +{
> +  error_t err = 0;
> +
> +  /*Create a clone of the root node of the translator. */
> +  unionmount_proxy = netfs_make_node (netfs_root_node->nn);

Would be nice to explain *why* it needs to be cloned...

> +  if (!unionmount_proxy)
> +    return ENOMEM;
> +
> +  /*Set the mountee on the proxy node. */
> +  /*Note that the flags parameter does not actually limit access to
> +    the translator's filesystem, because unionmount gives off ports to
> +    libnetfs nodes only when it looks up directories. The end client
> +    communicates to the translator when operating on regular files. */

I'm not sure I would understand this comment if I didn't already know
what it means...

> +  unionmount_start_mountee (diruser, unionmount_proxy, mountee_argz,
> +                         mountee_argz_len, O_READ, &mountee_port);

You forgot to assign "err".

> +
> +  mountee_started = 1;
> +
> +  return err;
> +}                            /* unionmount_setup */
> +
> diff --git a/unionmount.h b/unionmount.h
> index a3f7588..fabb2f5 100644
> --- a/unionmount.h
> +++ b/unionmount.h
> @@ -22,10 +22,36 @@
>  #ifndef INCLUDED_UNIONMOUNT_H
>  #define INCLUDED_UNIONMOUNT_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;
>  
> +/*The node the mountee is sitting on. */
> +extern node_t * unionmount_proxy;
> +
> +/*The port to the mountee. */
> +extern mach_port_t mountee_port;
> +
> +/*Shows whether the mountee has been started already. */
> +extern int mountee_started;
> +
> +/*Starts the mountee (given by `argz` and `argz_len`), sets it on node
> +  `np` and opens a port `port` to with `flags`. */
> +error_t
> +unionmount_start_mountee (struct protid * diruser, 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. */
> +/*Diruser is required for additional information. */
> +error_t
> +unionmount_setup (struct protid * diruser);
> +
>  #endif /*INCLUDED_UNIONMOUNT_H*/
> -- 
> 1.5.2.4
> 
> 

BTW, I do not see any actual proxying happening anywhere... Am I missing
something, or haven't you implemented that part yet?

-antrik-