[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: error checking in libnetfs
From: |
Neal H Walfield |
Subject: |
Re: error checking in libnetfs |
Date: |
02 Apr 2002 09:02:33 -0500 |
User-agent: |
Gnus/5.0808 (Gnus v5.8.8) Emacs/21.1 |
> > > 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.