Thank you. I applied this to master, adding the following NEWS entry:
- FreeBSD is now a supported platform, thanks to code contributions from
Gaetano Catalli, Ed Maste, and Giuseppe Lettieri.
On Wed, Jul 25, 2012 at 11:21:07PM +0200, Giuseppe Lettieri wrote:
> 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
>
>
> 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.letti...@iet.unipi.it
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
