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

Giuseppe Lettieri g.lettieri at iet.unipi.it
Wed Jul 25 21:21:07 UTC 2012


Hi,

here is the new revision of the patch, taking care of your comments below as discussed in the list. The main author is Catalli, and it contains work by Ed Maste and myself. The license has been updated to apache 2.0.

Regards,
Giuseppe

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-netdev-implementation-for-FreeBSD.patch
Type: application/octet-stream
Size: 66509 bytes
Desc: not available
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20120725/c528a8b9/attachment-0005.obj>
-------------- next part --------------

Il giorno 11/lug/2012, alle ore 01:59, Ben Pfaff ha scritto:

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


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



More information about the dev mailing list