Re: error checking in libnetfs

[Neal H Walfield]

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

I thought you meant outstanding as in need to update it for these
changes.  Sorry.

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

If ftpfs_dir_create fails, you need to free super_root.  And if
ftpfs_dir_create fails, you need to free super_root_dir.

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

Right, sorry about that.

> > > 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
> > > +      *newporttp = MACH_MSG_TYPE_MAKE_SEND;
> > > +      ports_port_deref (newpi);
> > 
> > Same idea here.
> > 
> 
>  What?

My mistake again.

> 
> > > +      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
> > > +      ports_port_deref (newpi);
> > 
> > As does this.
> 
>  In the error case newpi is not created.

Right.

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

This is the original:

  newpi = netfs_make_protid (user->po, new_user);
  if (newpi)
    {
      *newport = ports_get_right (newpi);
      mutex_unlock (&user->po->np->lock);
      *newporttype = MACH_MSG_TYPE_MAKE_SEND;
    }
  else
    {
      mutex_unlock (&user->po->np->lock);
      iohelp_free_iouser (new_user);
      err = ENOMEM;
    }
  

You changed it to:

  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;
    }
  mutex_unlock (&user->po->np->lock);
  iohelp_free_iouser (new_user);

i.e. you unconditionally free NEW_USER.





If you can change these things and resubmit a new patch, that would be
great.