--- Neal H Walfield <[EMAIL PROTECTED]> 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
_______________________________________________
Bug-hurd mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-hurd
