On Tue, Apr 13, 2010 at 02:54:23PM +0300, Antti Kantee wrote:
> On Tue Apr 13 2010 at 01:02:44 +0000, Adam Hoka wrote:
> > Module Name:        src
> > Committed By:       ahoka
> > Date:               Tue Apr 13 01:02:43 UTC 2010
> > 
> > Modified Files:
> >     src/sys/dist/pf/net: pf_ioctl.c
> > 
> > Log Message:
> > Do not auto unload pf if it's enabled.
> 
> Well what happens if you non-auto unload it when it's enabled?  Just let
> unload fail if it's busy.

There are a few issues with this:

- Unload should be "atomic".  If it fails for some reason, we must not
  urinate in the user's corkflakes. So on that basis the EBUSY check
  belongs in the CMD_UNLOAD block.

- At the point this is called, only module_lock is held.  So there
  is no locking in at least the autounload case noted.  Presumably the
  same is true for unload?  That could cause crashes and is therefore
  unacceptable.  (A strong choice of words - I am not trying to frustrate
  or rile anyone.  As we say in Dublin the truth does be bitter. :-)

- ENOTTY implies "I don't understand what you just asked", zero would
  be a better return code as in "yes that's fine, whatever". :-)

This all needs to be written down somewhere, I'll have a look.

> >  #ifdef _KERNEL_OPT
> >  #include "opt_inet.h"
> > @@ -3367,6 +3367,13 @@
> >             pfdetach();
> >             pflogdetach();
> >             return devsw_detach(NULL, &pf_cdevsw);
> > +   case MODULE_CMD_AUTOUNLOAD:
> > +           /* Do not auto unload pf if it's enabled. */
> > +           if (pf_status.running) {
> > +                   return EBUSY;
> > +           } else {
> > +                   return ENOTTY;
> > +           }
> >     default:
> >             return ENOTTY;
> >     }

Reply via email to