Subject: Re: Try again, itojun, patches need more work.
To: Darren Reed <avalon@caligula.anu.edu.au>
From: None <itojun@iijlab.net>
List: tech-net
Date: 06/30/2003 11:05:16
>> 	or i can forget about pfrdr variable for now, and change pf to
>> 	use pfil(9).  then you will be happy.
>
>So what this "pfrdr" does, no thanks to the lack of appropriate comments,
>is allow redirects back out the same interface a packet was received from
>without generating an ICMP redirect.  Reviewing this, the comment header
>for ip_forward() should be updated to indicate the *real* role of srcrt
>(and that is to prevent redirects from being issued) and the code in
>ip_input() should....well, if deemed useful it should go around the
>pfil(9) hooks BUT putting an IP address in an "int" just seems so very
>very wrong!  Maybe I'm spending too much time with C++ these days.  In
>general, that seems like a useful change to make, just its nature seems
>offensive.

	pfrdr is in_addr_t, not int.

	anyways, after some thinking pfrdr does not make sense to me as it
	is checking for overwritten ip_dst (rewritten), not the alternate route.
	if it were to notify ip_forward() the use of "route-to" (alternate
	nexthop) PF directive, it makes sense.  so forget about it, now my
	patch uses pfil(9).

>On the API issues, moving the pf*() functions into uipc_mbuf2.c does
>not seem like a long term solution, am I right ?

	for netbsd, i suppose it is a long term solution, for tagname-to-tag
	/reverse mapping.  now you can make changes to ipfilter so that
	ipfilter can query tag value (u_int16_t) from tagname, or vice versa,
	without requiring sys/net/pf.c.

>Or will you be merging those changes back into OpenBSD but have not
>yet ?
>
>I look in the pf code within the patch and I see #ifdef's to not use
>the uipc_mbuf2.c routines for OpenBSD.  There should be no need for
>those #ifdef's if you've settled on the API and both OS's are going
>to start using the same code.

	i don't think openbsd would take this API back to them, as they
	do not need it.
	i'm just keeping #ifdef __OpenBSD__ to make my work easier.  do not
	need to read between lines...

>On a separate note, a standard part of all internal kernel API's is
>a section 9 man page describing how they're used.
>
>Is this "API" in the patch you've posted a final API or is it known
>to be an interim measure and will be reviewed "soonish" ?  Kenjiro's
>comments support the latter and you've given no comment either way.
>If Kenjiro is the authority here then until that is finalised there
>should be no patches/diffs or merging.

	for tagname-to-tag/reverse, you can take it as my final proposal.
	for ALTQ portion, wait for kjc to respond.

itojun