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

[Giuseppe Lettieri]

Thanks Ben,

all of your comments look sensible and useful to me. I add some reply below.

Il 11/07/2012 01:59, Ben Pfaff ha scritto:
> Thanks.
> 
> Would you mind updating NOTICE and debian/copyright.in to mention the
> license on the new contribution?
>

Sure.

> 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.
>

I am a bit undecided about this. The installation is indeed pretty much
the same as described in INSTALL.Linux (except for the kernel module, of
course). But then FreeBSD only supports the userlevel datapath_type,
which is now in INSTALL.userlevel, and also needs some firewall rule to
avoid duplicates. Maybe an INSTALL.FreeBSD would be less noisy.


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

Sure.

> 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.
>

The VLOG thing is entirely my fault. For the other points maybe Gaetano
(in cc) may add some comment.


Giuseppe

-- 
Dr. Ing. Giuseppe Lettieri
Dipartimento di Ingegneria della Informazione
Universita' di Pisa
Largo Lucio Lazzarino 2, 56122 Pisa - Italy
Ph. : (+39) 050-2217.649 (direct) .599 (switch)
Fax : (+39) 050-2217.600
e-mail: g.lettieri at iet.unipi.it