Re: The patch of pfinet

[olafBuddenhagen]

Hi,

On Fri, Aug 08, 2008 at 08:09:24AM +0200, zhengda wrote:

> The patch enables the pfinet to work with the multiplexer and use the
> filter rule that only accepts the packet whose destination  is the
> pfinet server.

This rather sounds like two totally orthogonal changes, that really
should go into two separate patches?

> -CFLAGS += -fno-strict-aliasing
> +CFLAGS += -fno-strict-aliasing -DPCAP_SUPPORT
> +
> +LDFLAGS += -lpcap

It is probably better not to enable this by default, to avoid problems
with the external dependency...

Of course, once you have autoconf checks/options for pcap, this won't be
an issue anymore :-)

BTW, HAVE_PCAP would be a more conventional name for this kind of thing
I believe...

> diff -urN pfinet.old/ethernet.c pfinet/ethernet.c
> --- pfinet.old/ethernet.c    2007-10-09 10:01:34.000000000 +0200
> +++ pfinet/ethernet.c    2008-08-08 07:03:03.000000000 +0200
> @@ -21,29 +21,23 @@
[...]
> #include <linux/netdevice.h>
> #include <linux/etherdevice.h>
> #include <linux/if_arp.h>
> -
> +#include <device/bpf.h>
>
> struct port_class *etherreadclass;

You dropped a blank line here... Please try to avoid such spurious
changes.

> +/* The BPF instruction allows IP and ARP packets */
> +static struct bpf_insn ether_filter[] =
[...]

Do I get it right that the static rule doesn't check the IP, and only
when PCAP support is present, a proper rule will be generated?

> @@ -87,14 +81,14 @@
> ethernet_thread (any_t arg)
> {
>   ports_manage_port_operations_one_thread (etherport_bucket,
> -                       ethernet_demuxer,
> -                       0);
> +                                           ethernet_demuxer,
> +                                           0);
>   return 0;
> }
>
> int
> ethernet_demuxer (mach_msg_header_t *inp,
> -          mach_msg_header_t *outp)
> +                  mach_msg_header_t *outp)
> {
>   struct net_rcv_msg *msg = (struct net_rcv_msg *) inp;
>   struct sk_buff *skb;

Please don't change the formatting of code, if you don't otherwise touch
that code.

(This also applies in many other places throughout the patch.)

> @@ -109,10 +103,11 @@
>     if (inp->msgh_local_port == edev->readptname)
>       dev = &edev->dev;
>
> +  fprintf (stderr, "pfinet receives a message\n");

Seems you forgot to remove a debugging statement here?...

(Also applies in a few more places.)

> -  err = get_privileged_ports (0, &master_device);
> -  if (err)
> -    error (2, err, "cannot get device master port");
> +  if (master_device_file)
> +    {
> +      master_device = file_name_lookup (master_device_file , 0 , 0);
> +      if (master_device == MACH_PORT_NULL)
> +        error (10, 0, "file_name_lookup %s", master_device_file);
> +    }
> +  else
> +    {
> +      err = get_privileged_ports (0, &master_device);
> +      if (err)
> +        error (2, err, "cannot get device master port");
> +    }

Is it really appropriate to use a different error code in the case of
using an alternate master device port?...

> diff -urN pfinet.old/pcap_filter.c pfinet/pcap_filter.c
> --- pfinet.old/pcap_filter.c    1970-01-01 01:00:00.000000000 +0100
> +++ pfinet/pcap_filter.c    2008-08-08 07:06:26.000000000 +0200
> @@ -0,0 +1,76 @@
> +/*
> +   Copyright (C) 1993,94,95,96,97,98,99,2000,01,02,2006,2008
> +   Free Software Foundation, Inc.
[...]
> +/* Written by Zheng Da.  */

This is contradictory: Either you wrote the file yourself, in which case
the copyright years are obviously wrong; or you based it on some other
file, in which case it is not written by you alone...

If you indeed based it on some other file, please state so explicitely,
to make it clear -- or just leave out the "written by" part alltogether.

In either case, don't put it in an extra comment. If you want to include
author information, do so right after the copyright statement, before
the licensce boilerplate.

> +  insn = (struct bpf_insn *) malloc ((program.bf_len + 1) * sizeof  
> (*insn));

Your mail client mangled the patch, inserting a spurious linebreak.
(Also happened in a few other places.)

If for some reason you can't fix the mail client, send the patches as
attachements instead. This should be avoided if possible, as it's less
convenient, but it tends to be more robust against broken mail
clients...

-antrik-