Subject: Re: ENOIOCTL fix for ioctl code
To: None <tech-kern@netbsd.org, current-users@netbsd.org>
From: Andrew Brown <atatat@atatdot.net>
List: current-users
Date: 03/13/2002 00:32:09
i've been asked to further explain what exactly it is that i "fixed"
and why, so here goes.

i posted a note about my patch to tech-kern

	http://mail-index.netbsd.org/tech-kern/2002/03/10/0007.html

which contained some details, so i'll expound on that here.

the problem i claim to be solving is most easily visible in
nullioctl(), the lince discipline ioctl routine for line disciplines
that don't have a specific ioctl routine.

it returns -1.  that *sounds* fine, and the code that calls it
(through the l_ioctl pointer) checks (this is the other problem) to
see that it got something less than zero (or greater than or equal to
zero) in order to decide whether to continue, or to return an "error".

the problem is that -1 is also ERESTART.  if i changed nullioctl() to
return ERESTART, i wouldn't have actually changed any functionality,
but it would immediately be apparent that it was wrong.  ioctl
routines, specifically those dealing with ttys, sometimes get
interrupted and genuinely generate an ERESTART error.  with the
current manner in which the code is structured, this will get ignored
and probably turned into ENOTTY (see ptyioctl() in tty_pty.c for one
instance of this).

the problem, then, is this: ioctl code paths have no formal way of
indicating the difference between an unhandled ioctl code (currently
either ENOTTY or -1, depending on the writer's whim), an error in
ioctl handling (eg, ERESTART), or an error in ioctl parameters
(EINVAL, EOPNOTSUPP, EAFNOSUPPORT, etc).

my solution to this problem comes in three pieces.

(1) move ERESTART from -1 to -3, which is a little arbitrary, but by
moving it away from -1 (a common "error" value), any more problems
that are distantly related ought to become more apparent [i've found
none so far].

(2) introduce the new pseudo error code ENOIOCTL, which will *only* be
used in the kernel.  at the bottom of sys_ioctl(), i check for an
error value of ENOIOCTL and convert it to ENOTTY for userland to see.
unlike ERESTART (and EJUSTRETURN), ENOIOCTL will *never* leave kernel
space.

(3) convert as much ioctl code as possible to return ENOIOCTL instead
of ENOTTY or -1 (and in some cases EINVAL), and alter the code path to
check for "error != ENOIOCTL" instead of "error < 0" or "error >= 0",
etc.  ERESTART and EJUSTRETURN are negative, but should be passed back
up as other errors, instead of being "ignored" as the code marches
merrily on.

anyway, that's what i did.  there were never many conflicts in the
code i changed, so two patches i've posted are largely the same.  i'm
currently removing the option i noted, and will then commit the code
i've changed.  the response that i've gotten so far is that everything
is fine, and the easily reproducible bugs are gone, so it seems fine.
if anyone else wants to test it and complain, please feel free.

-- 
|-----< "CODE WARRIOR" >-----|
codewarrior@daemon.org             * "ah!  i see you have the internet
twofsonet@graffiti.com (Andrew Brown)                that goes *ping*!"
andrew@crossbar.com       * "information is power -- share the wealth."