Re: error checking in libnetfs

[Neal H Walfield]

>   Here is my patch to increase the error checking within libnetfs.
> I've also included error.h in a couple of files that I noticed had compiler
> warnings.  There aren't any functions that return pointers to struct's
> left in libnetfs.

There is one in libdiskfs (diskfs_make_node).  Do you want to fix this
too?

>   Apparently the copyright wasn't updated in console-run.c during the last
> set of checkins.
> 
>   Just a reminder, there are outstanding documentation patches for hurd.texi.

Could you write these?

> nfs:
>       * nfs.h: Changed lookup_fhandle to return an error_t instead
> of void.

I think you should do:

        * nfs.h (lookup_fhandle): Now returns error_t instead of void.

> nfsd:
>       * fsys.c: Include error.h, so that the error function is explicitly
>       declared.

This should read:

        * fsys.c: Include <error.h>.

You need to <>s and there is no need for an explanation in this case.

> Index: ftpfs/fs.c
> ===================================================================
> RCS file: /cvsroot/hurd/hurd/ftpfs/fs.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 fs.c
> --- ftpfs/fs.c        29 Dec 2001 00:19:39 -0000      1.4
> +++ ftpfs/fs.c        29 Mar 2002 06:12:05 -0000
> @@ -64,10 +64,8 @@ ftpfs_create (char *rmt_path, int fsid,
>  
>    if (! err)
>      {
> -      super_root = netfs_make_node (0);
> -      if (! super_root)
> -     err = ENOMEM;
> -      else
> +      err = netfs_make_node (0, &super_root);
> +      if (! err)
>       {
>         err = ftpfs_dir_create (new, super_root, rmt_path, &super_root_dir);
>         if (! err)

This seems wrong: you ignore netfs_make_node's error case!

> Index: libnetfs/fsys-getroot.c
> ===================================================================
> RCS file: /cvsroot/hurd/hurd/libnetfs/fsys-getroot.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 fsys-getroot.c
> --- libnetfs/fsys-getroot.c   16 Jun 2001 20:23:29 -0000      1.10
> +++ libnetfs/fsys-getroot.c   29 Mar 2002 06:12:07 -0000
> @@ -127,20 +128,28 @@ netfs_S_fsys_getroot (mach_port_t cntl,
>    
>    flags &= ~OPENONLY_STATE_MODES;
>    
> -  newpi = netfs_make_protid (netfs_make_peropen (netfs_root_node, flags,
> -                                              &peropen_context),
> -                          cred);
> -  mach_port_deallocate (mach_task_self (), dotdot);
> -
> -  *do_retry = FS_RETRY_NORMAL;
> -  *retry_port = ports_get_right (newpi);
> -  *retry_port_type = MACH_MSG_TYPE_MAKE_SEND;
> -  retry_name[0] = '\0';
> -  ports_port_deref (newpi);
> -  
> +  err = netfs_make_peropen (netfs_root_node, flags, &peropen_context, 
> &newpo);
> +  if (! err)
> +    {
> +      err = netfs_make_protid (newpo, cred, &newpi);
> +      if (err)
> +     netfs_release_peropen (newpo);
> +    }
> +
> +  if (! err)
> +    {
> +      mach_port_deallocate (mach_task_self (), dotdot);

I think this needs to happen no matter what (i.e. error or not).

> +      
> +      *do_retry = FS_RETRY_NORMAL;
> +      *retry_port = ports_get_right (newpi);
> +      *retry_port_type = MACH_MSG_TYPE_MAKE_SEND;
> +      retry_name[0] = '\0';
> +      ports_port_deref (newpi);

So does this.

> +    }
> +  else
>   out:
> -  if (err)
>      iohelp_free_iouser (cred);
> +
>    mutex_unlock (&netfs_root_node->lock);
>    return err;
>  }
> Index: libnetfs/io-duplicate.c
> ===================================================================
> RCS file: /cvsroot/hurd/hurd/libnetfs/io-duplicate.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 io-duplicate.c
> --- libnetfs/io-duplicate.c   16 Jun 2001 20:23:29 -0000      1.3
> +++ libnetfs/io-duplicate.c   29 Mar 2002 06:12:07 -0000
> @@ -35,10 +35,15 @@ netfs_S_io_duplicate (struct protid *use
>      return err;
>    
>    mutex_lock (&user->po->np->lock);
> -  newpi = netfs_make_protid (user->po, clone);
> -  *newport = ports_get_right (newpi);
> -  mutex_unlock (&user->po->np->lock);
> -  *newporttp = MACH_MSG_TYPE_MAKE_SEND;
> -  ports_port_deref (newpi);
> -  return 0;
> +  err = netfs_make_protid (user->po, clone, &newpi);
> +  if (! err)
> +    {
> +      *newport = ports_get_right (newpi);
> +      mutex_unlock (&user->po->np->lock);

You need to always unlock USER->po->np->lock.  Not just on success.

> +      *newporttp = MACH_MSG_TYPE_MAKE_SEND;
> +      ports_port_deref (newpi);

Same idea here.

> +      return 0;
> +    }
> +  else
> +    return err;
>  }
> Index: libnetfs/io-reauthenticate.c
> ===================================================================
> RCS file: /cvsroot/hurd/hurd/libnetfs/io-reauthenticate.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 io-reauthenticate.c
> --- libnetfs/io-reauthenticate.c      16 Jun 2001 20:23:29 -0000      1.10
> +++ libnetfs/io-reauthenticate.c      29 Mar 2002 06:12:07 -0000
> @@ -1,5 +1,5 @@
>  /*
> -   Copyright (C) 1995,96,2000,01 Free Software Foundation, Inc.
> +   Copyright (C) 1995,96,2000,01,02 Free Software Foundation, Inc.
>     Written by Michael I. Bushnell, p/BSG.
>  
>     This file is part of the GNU Hurd.
> @@ -32,22 +32,28 @@ netfs_S_io_reauthenticate (struct protid
>      return EOPNOTSUPP;
>  
>    mutex_lock (&user->po->np->lock);
> -  newpi = netfs_make_protid (user->po, 0);
> +  err = netfs_make_protid (user->po, 0, &newpi);
>  
> -  newright = ports_get_send_right (newpi);
> -  assert (newright != MACH_PORT_NULL);
> +  if (! err)
> +    {
>  
> -  err = iohelp_reauth (&newpi->user, netfs_auth_server_port, rend_port,
> -                    newright, 1);
> -
> -  mach_port_deallocate (mach_task_self (), rend_port);
> -  mach_port_deallocate (mach_task_self (), newright);
> -
> -  mach_port_move_member (mach_task_self (), newpi->pi.port_right,
> -                      netfs_port_bucket->portset);
> -
> -  mutex_unlock (&user->po->np->lock);
> -  ports_port_deref (newpi);
> -
> -  return err;
> +      newright = ports_get_send_right (newpi);
> +      assert (newright != MACH_PORT_NULL);
> +      
> +      err = iohelp_reauth (&newpi->user, netfs_auth_server_port, rend_port,
> +                        newright, 1);
> +      
> +      mach_port_deallocate (mach_task_self (), rend_port);
> +      mach_port_deallocate (mach_task_self (), newright);
> +      
> +      mach_port_move_member (mach_task_self (), newpi->pi.port_right,
> +                          netfs_port_bucket->portset);
> +      
> +      mutex_unlock (&user->po->np->lock);

This needs to be called in both cases, i.e. error and success.

> +      ports_port_deref (newpi);

As does this.

> +
> +      return err;
> +    }
> +  else
> +    return err;
>  }
> Index: libnetfs/io-restrict-auth.c
> ===================================================================
> RCS file: /cvsroot/hurd/hurd/libnetfs/io-restrict-auth.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 io-restrict-auth.c
> --- libnetfs/io-restrict-auth.c       16 Jun 2001 20:37:39 -0000      1.4
> +++ libnetfs/io-restrict-auth.c       29 Mar 2002 06:12:07 -0000
> @@ -1,5 +1,5 @@
>  /* 
> -   Copyright (C) 1995,96,2001 Free Software Foundation, Inc.
> +   Copyright (C) 1995,96,2001,2002 Free Software Foundation, Inc.
>     Written by Michael I. Bushnell, p/BSG.
>  
>     This file is part of the GNU Hurd.
> @@ -96,21 +96,17 @@ netfs_S_io_restrict_auth (struct protid 
>      }
>    
>    mutex_lock (&user->po->np->lock);
> -  newpi = netfs_make_protid (user->po, new_user);
> -  if (newpi)
> +  err = netfs_make_protid (user->po, new_user, &newpi);
> +  if (! err)
>      {
>        *newport = ports_get_right (newpi);
> -      mutex_unlock (&user->po->np->lock);
>        *newporttype = MACH_MSG_TYPE_MAKE_SEND;
> +      ports_port_deref (newpi);
>      }
> -  else
> -    {
> -      mutex_unlock (&user->po->np->lock);
> -      iohelp_free_iouser (new_user);
> -      err = ENOMEM;
> -    }
> -  
> -  ports_port_deref (newpi);
> +  mutex_unlock (&user->po->np->lock);
> +
> +  iohelp_free_iouser (new_user);

This change looks wrong.  new_user should not be freed in the success
case.  Also, ports_port_deref needs to happen after the unlock.

>    return err;
> +  
>  }
>  
> Index: libnetfs/netfs.h
> ===================================================================
> RCS file: /cvsroot/hurd/hurd/libnetfs/netfs.h,v
> retrieving revision 1.29
> diff -u -p -r1.29 netfs.h
> --- libnetfs/netfs.h  30 Jan 2001 00:50:25 -0000      1.29
> +++ libnetfs/netfs.h  29 Mar 2002 06:12:08 -0000
> @@ -263,11 +263,6 @@ error_t netfs_attempt_write (struct ious
>  error_t netfs_report_access (struct iouser *cred, struct node *np,
>                            int *types);
>  
> -/* The user must define this function.  Create a new user from the
> -   specified UID and GID arrays. */
> -struct iouser *netfs_make_user (uid_t *uids, int nuids,
> -                                    uid_t *gids, int ngids);
> -
>  /* The user must define this function.  Node NP has no more references;
>     free all its associated storage. */
>  void netfs_node_norefs (struct node *np);
> @@ -341,9 +336,9 @@ extern int netfs_maxsymlinks;
>  /* Definitions provided by netfs. */
>  
>  /* Given a netnode created by the user program, wraps it in a node
> -   structure.  The new node is not locked and has a single reference.
> -   If an error occurs, NULL is returned.  */
> -struct node *netfs_make_node (struct netnode *);
> +   structure, *nnp.  The new node is not locked and has a single
> reference.

Local variables and arguments must be in caps.  I.e. NNP.

> +   If an error occurs, the error value is returned.  */
> +error_t netfs_make_node (struct netnode *nn, struct node **nnp);
>  /* Whenever node->references is to be touched, this lock must be
>     held.  Cf. netfs_nrele, netfs_nput, netfs_nref and netfs_drop_node.  */
> @@ -363,15 +358,16 @@ mach_port_t netfs_startup (mach_port_t b
>  void netfs_server_loop (void);
>  
>  /* Creates a new credential from USER which can be NULL on the peropen
> -   PO.  Returns NULL and sets errno on error.  */
> -struct protid *netfs_make_protid (struct peropen *po, struct iouser *user);
> +   PO.  Returns the error value.  */
> +error_t netfs_make_protid (struct peropen *po, struct iouser *user, 
> +                        struct protid **pi);
>  
> -/* Create and return a new peropen structure on node NP with open
> +/* Create a new peropen structure, *ppo, on node NP with open

Likewise for PPO.

>     flags FLAGS.  The initial values for the root_parent, shadow_root,
>     and shadow_root_parent fields are copied from CONTEXT if it's
> -   non-zero, otherwise zeroed.  */
> -struct peropen *netfs_make_peropen (struct node *, int,
> -                                 struct peropen *context);
> +   non-zero, otherwise zeroed.  Returns the error value. */
> +error_t netfs_make_peropen (struct node *np, int, struct peropen *context,
> +                         struct peropen **ppo);
>  
>  /* Add a reference to node NP. Unless you already hold a reference,
>     NP must be locked.  */
> Index: trans/firmlink.c
> ===================================================================
> RCS file: /cvsroot/hurd/hurd/trans/firmlink.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 firmlink.c
> --- trans/firmlink.c  26 Feb 2001 04:16:01 -0000      1.12
> +++ trans/firmlink.c  29 Mar 2002 06:12:17 -0000
> @@ -1,8 +1,9 @@
>  /* A translator for `firmlinks'
>  
> -   Copyright (C) 1997, 1998, 1999, 2001 Free Software Foundation, Inc.
> +   Copyright (C) 1997, 1998, 1999, 2001, 2002 Free Software Foundation, Inc.
>  
>     Written by Miles Bader <miles@gnu.ai.mit.edu>
> +   Extended by James A. Morrison <ja2morri@uwaterloo.ca>
>  
>     This program is free software; you can redistribute it and/or
>     modify it under the terms of the GNU General Public License as
> @@ -26,42 +27,91 @@
>  #include <fcntl.h>
>  #include <argp.h>
>  #include <error.h>
> +#include <time.h>
>  #include <sys/mman.h>
>  
>  #include <hurd/trivfs.h>
>  
>  #include <version.h>
>  
> +#define TIME_MODE 0
> +#define RANDOM_MODE 1
> +#define SEQUENCE_MODE 2
> +
>  const char *argp_program_version = STANDARD_HURD_VERSION (firmlink);
>  
>  static const struct argp_option options[] =
>  {
> +  {"randomized", 'r', NULL, 0, "Randomize selection of files, default" },
> +  {"sequencial", 's', NULL, 0, "Choose files sequencially" },
> +  {"format",'f',"DATE FORMAT", 0,
> +   "Create files named file1/file2.DATE FORMAT"},
>    { 0 }
>  };
>  
> -static const char args_doc[] = "TARGET";
> -static const char doc[] = "A translator for firmlinks."
> +static const char args_doc[] = "[file1 file2 ...]";
> +static const char doc[] = "A translator for multiple firmlinks."
>  "\vA firmlink is sort of half-way between a symbolic link and a hard link:"
>  "\n"
>  "\nLike a symbolic link, it is `by name', and contains no actual reference 
> to"
>  " the target.  However, the lookup returns a node which will redirect parent"
>  " lookups so that attempts to find the cwd that go through the link will"
>  " reflect the link name, not the target name.  The target referenced by the"
> -" firmlink is looked up in the namespace of the translator, not the client.";
> +" firmlink is looked up in the namespace of the translator, not the client."
> +" A multiple firmlink can point to more than one file, and can also create"
> +" files based on dates.";
>  
>  /* Link parameters.  */
> -static char *target = 0;     /* What we translate too.  */
> +
> +struct arg_struct 
> +{
> +  char **args;
> +  char *date_format;
> +  int type;
> +  int flags;
> +  unsigned long lastused;
> +  unsigned long size;
> +};
> +
> +struct arg_struct config; 
>  
>  /* Parse a single option/argument.  */
>  static error_t
>  parse_opt (int key, char *arg, struct argp_state *state)
>  {
> -  if (key == ARGP_KEY_ARG && state->arg_num == 0)
> -    target = arg;
> -  else if (key == ARGP_KEY_ARG || key == ARGP_KEY_NO_ARGS)
> -    argp_usage (state);
> -  else
> -    return ARGP_ERR_UNKNOWN;
> +  struct arg_struct *arguments = state->input;
> +
> +  switch (key) 
> +    {
> +    case 'r':
> +      arguments->type = RANDOM_MODE;
> +      arguments->flags = O_CREAT;
> +      break; 
> +    case 's':
> +      arguments->type = SEQUENCE_MODE;
> +      arguments->flags = O_CREAT;
> +      arguments->lastused = 0;
> +      break;
> +    case 'f':
> +      arguments->type = TIME_MODE;
> +      arguments->date_format = arg;
> +      arguments->flags = 0;
> +      break;
> +    case ARGP_KEY_ARG:
> +      arguments->args[arguments->size] = arg;
> +      arguments->size++;
> +      break;
> +    case ARGP_KEY_END:
> +      if ( arguments->type == TIME_MODE && arguments->size != 2 )
> +     argp_usage (state);
> +      break; 
> +    case ARGP_KEY_NO_ARGS:
> +      argp_usage (state);
> +      break;
> +    default:
> +      return ARGP_ERR_UNKNOWN;
> +    }
> +  

Please put this in a seperate different patch.  Not to mention, there
are no change log entries indicating these changes.

>    return 0;
>  }
>  
> @@ -74,8 +124,16 @@ main (int argc, char **argv)
>    mach_port_t bootstrap;
>    struct trivfs_control *fsys;
>  
> +  config.size = 0;
> +  config.type = RANDOM_MODE;
> +  config.args = (char**) malloc (argc);
> +  if (! config.args)
> +    error (ENOMEM, 1, "Starting up");
> +
>    /* Parse our options...  */
> -  argp_parse (&argp, argc, argv, 0, 0, 0);
> +  argp_parse (&argp, argc, argv, 0, 0, &config);
> +  if ( config.type == RANDOM_MODE ) 
> +    srand (time (NULL) );

Check these parentheseses.

>    task_get_bootstrap_port (mach_task_self (), &bootstrap);
>    if (bootstrap == MACH_PORT_NULL)
> @@ -102,7 +160,26 @@ firmlink (mach_port_t parent, const char
>  {
>    error_t err;
>    file_t authed_link;
> -  file_t target = file_name_lookup (target_name, flags & ~O_CREAT, 0);
> +  file_t target = file_name_lookup (target_name, flags & ~config.flags, 0);