[ovs-dev] [PATCH] netdev implementation for FreeBSD

[Ben Pfaff]

On Tue, Jul 10, 2012 at 03:22:00PM +0200, Giuseppe Lettieri wrote:
> here is a new version of the FreeBSD porting patch, signed-off by Catalli.
> 
> W.r.t. the previous patch, I only replaced a printf with a VLOG_WARN_RL
> and declared a COVERAGE counter (as suggest by Ben below), and removed
> some trailing white space to make git-apply happy.

Thanks.

This looks very good.  I have a few comments.

Would you mind updating NOTICE and debian/copyright.in to mention the
license on the new contribution?

Do you want to add an INSTALL.FreeBSD or similar file at top level?
Feel free to copy any applicable text from INSTALL.Linux.  Or, if you
think it's better, please feel free to edit INSTALL.Linux to include
FreeBSD instructions also, and then we can rename it to just INSTALL.

In netdev-bsd.c, please #include <config.h> before anything else.

The PCAP_SNAPLEN of 1024 surprises me.  Wouldn't you want at least
1500, so that you can capture full Ethernet frames?  (Maybe 1514 or
1518?  I don't know whether the snaplen includes the Ethernet and VLAN
headers.)

In netdev_bsd_listen(), does pcap_open_live() usefully set errno?
Perhaps we should log the error text that it reports when it fails.

In proc_pkt(), this log message is misformatted in various ways:
        VLOG_WARN_RL(&rl, "%s Warning: Packet truncated'n", __func__);
The function name shouldn't be needed, and neither should the string
"Warning" (since the message is already a warning).  I guess that 'n
is a typo for \n, but VLOG_WARN_RL will supply the new-line itself.
So, I'd probably change it to just:
        VLOG_WARN_RL(&rl, "packet truncated");

It's a little sad that the pcap architecture means that we have to do
an extra copy of the packet.

My docs for libpcap mention a "pcap_next_ex" function that can be used
to distinguish errors from no available packets.  That might be easier
to use than pcap_dispatch().

It would be a good idea to update netdev_bsd_send() to fix the issue
mentioned by the comment:

    /* XXX should support sending even if 'ethertype' was NETDEV_ETH_TYPE_NONE.
     */

because according to commit 76c308b50d (netdev-linux: Support 'send'
for netdevs opened with NETDEV_ETH_TYPE_NONE.), which fixed the same
issue in netdev-linux, bonding will not work without this issue being
fixed.

There's a line of commented-out code in netdev_bsd_get_stats():
    //COVERAGE_INC(netdev_get_stats);
Also in set_etheraddr():
    //COVERAGE_INC(netdev_set_hwaddr);

Let me know what you think.

Thanks,

Ben.