bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: error checking in libnetfs


From: James A Morrison
Subject: Re: error checking in libnetfs
Date: Tue, 2 Apr 2002 00:14:21 -0500 (EST)

--- Neal H Walfield <neal@cs.uml.edu> wrote:
> >   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?
> 

 Yup, I can do that.

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

 Write what?  I can write more documentation if you like ;)

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

Thanks.

> > 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!
> 

 What?  If an error occurs, the malloced space is free'd and the value in err
is returned.

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

 You are right.

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

 It doesn't in libdiskfs/fsys-getroot.c .

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

 Yup, fixed.

> > +      *newporttp = MACH_MSG_TYPE_MAKE_SEND;
> > +      ports_port_deref (newpi);
> 
> Same idea here.
> 

 What?

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

 Yup.

> > +      ports_port_deref (newpi);
> 
> As does this.

 In the error case newpi is not created.

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

 Humm, again I followed the example in libdiskfs/io-restrict-auth.c .

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

 Thanks.

 Sorry, the firmlink stuff wasn't supposed to be in the patch, somehow I 
managed to keep it out of every patch but the one I posted.

James A. Morrison










reply via email to

[Prev in Thread] Current Thread [Next in Thread]