Re: The patch of pfinet

[olafBuddenhagen]

Hi,

On Tue, Aug 12, 2008 at 03:52:58PM +0200, zhengda wrote:

> pfinet1.patch. To make it open the virtual network interface.
> pfinet2.patch. To use the proper filter rule in pfinet.
> pfinet3.patch. To enable ioctl to set the network device into the  
> promiscuous mode.

Would be nice for the file names to include a hint what the pathes do,
i.e. something like pfinet-virt_iface.patch, pfinet-use_IP_filter.patch,
pfinet-promiscuous_mode.patch :-)

> +      master_device = file_name_lookup (master_device_file , 0 , 0);
> +      if (master_device == MACH_PORT_NULL)
> +        error (2, 0, "file_name_lookup %s", master_device_file);

Doesn't file_name_lookup() set errno?...

>         for (in = h->interfaces; in < h->interfaces + h->num_interfaces; in++)
> -         if (strcmp (in->device->name, arg) == 0)
> +            if (strcmp (in->name, arg) == 0)

Wrong indentation.

> +#ifdef HAVE_PCAP
[...]
> +#else
> +
> +int ethernet_reset_ipfilter (struct device *dev, struct in_addr addr)
> +{
> +  return 0;
> +}
> +
> +#endif

When using long #ifdef blocks, please comment the #else/#endif
statements -- see the GNU Coding Standards.

> +      err = dev_change_flags(dev, flags);
> +      err = machdev_change_flags (dev, flags);

The second call will overwrite the err returned by the first... You need
another condition here.

-antrik-