IPF in our source tree

Hi,

While reading the IPF source code (in preparation for 4.1.23
import) I noticed
few things which I don't understand:

- Why did we perform s/ipfattach/iplattach/ and
s/ipfdetach/ipldetach/ ?
   Any objections if I revert back to the original version?

- Why was the use of caddr_t deprecated? I'm fine with
void*, I'd just like to
   know the technical reason for this...

- In some files under src/sys/dist/ipf/netinet the original
code checks
   the return value from BCOPYIN() but we have removed those
checks. Why?
   Any objections if I revert back to the original version?

- Some fuctions (e.g. fr_sttab_destroy) were converted from
the old K&R(?)
   style to the new ANSI(?) C style. Why? There are still
functions using
   the old style. Any objections if I revert back to the
original version?

The reason I'm asking these is because I'd like to keep the
number of changes
as small as possible (and feed all our changes to Darren so
he can include
them in the official IPF code). This makes my life as the
IPF maintainer much
easier...

Martti

On Fri, Jun 01, 2007 at 12:59:22PM +0300, Martti Kuparinen
wrote:
> - In some files under src/sys/dist/ipf/netinet the
original code checks
>   the return value from BCOPYIN() but we have removed
those checks. Why?
>   Any objections if I revert back to the original
version?

That is not possible. The idiom used by Darren does not work
for NetBSD,
it can not even compile (unless you paper over it with
gcc'isms and make
the macros always return 0, which is not a lot better than
the current
state).

I suggested to Darren to change the style from

  int err = 0;
  err = BCOPYIN(some, args);

to

  int err = 0;
  BCOPYIN(some, args, err);

But this should be fixed upstream, not in our import.

Martin

Martin Husemann wrote:

> No. COPYIN is the alias for copyin(), but BCOPYIN is
memcpy(), which does
> not return an error value.

Oh, you are right. I'll talk to Darren about this.

PS. I'd like to hear comments for the remaining questions as
well...

Martti

Martin Husemann wrote:
> On Fri, Jun 01, 2007 at 12:59:22PM +0300, Martti
Kuparinen wrote:
>> - In some files under src/sys/dist/ipf/netinet the
original code checks
>>   the return value from BCOPYIN() but we have
removed those checks. Why?
>>   Any objections if I revert back to the original
version?
> 
> That is not possible. The idiom used by Darren does not
work for NetBSD,
> it can not even compile (unless you paper over it with
gcc'isms and make
> the macros always return 0, which is not a lot better
than the current
> state).

I don't understand this, copyin() is a normal function which
takes 3 arguments 
and returns an int. The BCOPYIN macro is just an
"alias" for copyin(), i.e. the 
preprosessor replaces BCOPYIN with copyin before the
compiler compiles the code.

Isn't this exactly the same case as with the following test
code?

n106:~> cat tryme.c
extern int copyin(const void *uaddr, void *kaddr, int len);

#define BCOPYIN(u, k, l) copyin((u), (k), (l))

int somename()
{
         char u[1], k[1];

         return BCOPYIN(&u, &k, sizeof(u));
}
n106:~> gcc -Wall -c tryme.c
n106:~>

On Fri, Jun 01, 2007 at 01:31:52PM +0300, Martti Kuparinen
wrote:
> I don't understand this, copyin() is a normal function
which takes 3 
> arguments and returns an int. The BCOPYIN macro is just
an "alias" for 
> copyin(), i.e. the preprosessor replaces BCOPYIN with
copyin before the 
> compiler compiles the code.

No. COPYIN is the alias for copyin(), but BCOPYIN is
memcpy(), which does
not return an error value. The ioctl framework already has
checked for
failures from copyin() in this case.

Martin

On Fri, Jun 01, 2007 at 12:59:22PM +0300, Martti Kuparinen
wrote:
> 
> - Some fuctions (e.g. fr_sttab_destroy) were converted
from the old K&R(?)
>   style to the new ANSI(?) C style. Why? There are
still functions using
>   the old style. Any objections if I revert back to the
original version?

The big problem with K&R style is it assumes everything
fits in an int. 
Since our LP64 systems are LP64I32, that means passing longs
and pointers 
is problematic.

To be honest, I know that's why K&R function
declarations are bad. I'm not 
100% sure what happens if you have an ANSI prototype and a
K&R function 
start.

So yes, I here by object to reverting to K&R functions.


Take care,

Bill

   
   - Why did we perform s/ipfattach/iplattach/ and
s/ipfdetach/ipldetach/ ?
      Any objections if I revert back to the original
version?


as i recall it had something to do with ipf vs. our autoconf
vs.
LKM's?  it's a really long time ago and if you can revert it
and
everything still works -- go for it.


.mrg.

On Fri, Jun 01, 2007 at 12:06:28PM +0200, Martin Husemann
wrote:
> That is not possible. The idiom used by Darren does not
work for NetBSD,
> it can not even compile (unless you paper over it with
gcc'isms and make
> the macros always return 0, which is not a lot better
than the current
> state).
> 
> I suggested to Darren to change the style from
> 
>   int err = 0;
>   err = BCOPYIN(some, args);
> 
> to
> 
>   int err = 0;
>   BCOPYIN(some, args, err);
> 

Why not just define the macro to return 0?  I don't see how
that's 
different from having the macro set a variable to 0.
#define BCOPYIN(a,b) (whatever(a,b), 0)
Is there something about this definition that's a gccism?

eric

>> [...K&R definitions vs prototype
definitions...]
> The big problem with K&R style is it assumes
everything fits in an
> int.

Not really; you can pass around large types in K&R
style.  What you
don't get is the implicit cast from the actual argument's
type to the
formal parameter's type.  Provided you're careful that the
actual
argument's type (after the default promotions) is compatible
with the
formal parameter's type (ditto), you're fine - whether or
not it fits
in an int.

Coders used to the implicit casts provided by prototype
declarations
are likely to find the necessary discipline difficult. :-/

> I'm not 100% sure what happens if you have an ANSI
prototype and a
> K&R function start.

I'd have to check with a real C maven to be sure.  But I
think that
provided the types in the prototype match the formal
parameter types,
and those types are compatible with what they turn into
after the
default promotions, you're OK - and that if this doesn't
hold, it is an
error (though I'm not sure whether you get
implementation-defined or
undefined behaviour).

In gcc, the prototype declaration sometimes silently
overrides the
old-style definition: "in GNU C, a function prototype
argument type
overrides the argument type specified by a later old-style
definition
if the former type is the same as the latter type before
promotion"
(quoted from the extend.texi from 3.1; the extend.texi from
1.4T has
wording that looks identical to an eyeball compare).  I'm
not sure
whether I think this is a feature or a misfeature.  (I think
the lack
of an option to disable it is definitely a misfeature.)

/~ The ASCII				der Mouse
 / Ribbon Campaign
 X  Against HTML	       mouserodents.montreal.qc.ca
/  Email!	     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3
27 4B

On Sat, Jun 02, 2007 at 12:42:54AM -0400, der Mouse wrote:
> >> [...K&R definitions vs prototype
definitions...]
> > The big problem with K&R style is it assumes
everything fits in an
> > int.
> 
> Not really; you can pass around large types in K&R
style.  What you
> don't get is the implicit cast from the actual
argument's type to the
> formal parameter's type.  Provided you're careful that
the actual

Uhm, that's why I said "assume" as opposed to
"must" or some other word. 


> argument's type (after the default promotions) is
compatible with the
> formal parameter's type (ditto), you're fine - whether
or not it fits
> in an int.
> 
> Coders used to the implicit casts provided by prototype
declarations
> are likely to find the necessary discipline difficult.
:-/

My understanding is that, back in the day, coders used to
the "necessary
discipline" found it difficult. Which is why we have
prototypes. 

Take care,

Bill