[ovs-dev] [PATCH] stuck with 16-bit ifr_flags in FreeBSD

[Ben Pfaff]

On Sat, Apr 04, 2015 at 12:10:30AM +0800, Kevin Lo wrote:
> On Fri, Apr 03, 2015 at 08:16:18AM -0700, Ben Pfaff wrote:
> > 
> > On Fri, Apr 03, 2015 at 10:02:16PM +0800, Kevin Lo wrote:
> > > I'm attempting to install ovs 2.3.1 on FreeBSD and it appears to be broken
> > > after the commit 666afb55e84e9118812de81a75655ec9567b7a5b.
> > > Since FreeBSD uses two short integers to represent interface flags, 
> > > we have to apply mask 0xffff to flags.
> > > 
> > > Signed-off-by: Kevin Lo <kevlo at FreeBSD.org>
> > 
> 
> CC: Ed Maste <emaste at freebsd.org>
> 
> > It's hard for me to see how your patch makes a difference.  If
> > ifr->ifr_flags is 16-bits, then ifr->ifr_flags and (ifr->ifr_flags &
> > 0xffff) have the same value, and assigning to ifr->ifr_flags has the
> > same effect whether you mask off high bits or not.
> 
> FreeBSD fills the int return value with ifr_flagshigh in the high
> 16 bits and ifr_flags in the low 16 bits rather than blindly promoting 
> ifr_flags to an int, which will preserve the sign.
> FreeBSD places it into the lower 16 bits of the return value.

You mean that the problem is sign extension in converting short to
int, so that if the high bit of ifr_flags is set then ifr_get_flags()
returns a corrupted value?  OK, I buy that.  Can you update the commit
message to say so?  I don't see how if_set_flags() could be affected
by this but it doesn't hurt to update that too.

> > Also, the commit you name was in 2013, and we've had other 
> > contributions from FreeBSD contributors since then 
> > (namely Ed Maste <emaste at freebsd.org>) and it seems like he
> > would have noticed if it were totally busted.
> 
> You mean this message:
> http://openvswitch.org/pipermail/dev/2013-May/027594.html
> 
> If so, Ed replied that he reviewed the patch but didn't apply it to test. :-)

Ed is the author of the following 37 commits in the Open vSwitch
repository.  Ed, did you really never test Open vSwitch on FreeBSD,
despite this long list of contributions?

    Ed Maste (37):
          lib: Do not assume sig_atomic_t is int.
          vlandev: Move Linux #include under #ifdef __linux__
          Route-table implementation for (Free)BSD
          Add forgotten header
          Avoid implementation-defined strerror behaviour
          Use int type for setsockopt IP_TOS value
          tests: Handle different output formats for 'wc -l'.
          tests: Make compatible with FreeBSD's sed.
          Don't assume python is in /usr/bin.
          tests: Avoid xargs, for FreeBSD compatibility.
          utilities: FreeBSD compatibility.
          tests: Get max rx socket buffer size on FreeBSD
          lib: Correct "old-style function definition" warning.
          lib: Add header #include for writev
          netdev-bsd: Initialize variable to silence a compiler warning.
          netdev: Map to OpenFlow port for flow lookup
          lib: Add xpipe_nonblocking helper
          tests: Also enable FreeBSD libc debugging
          tests: jemalloc debug config for FreeBSD 9 and 10.
          lib: Move addition of program_name to proctitle_set
          lib: Use FreeBSD libc function for proctitle
          lib: Accomodate FreeBSD return value for ssl connection.
          lib: remove duplicate #include <config.h>
          Prevent pager from appearing during build
          netdev-bsd: Use underlying tap device on netdev_bsd_listen().
          vswitchd: Avoid writing to const struct
          netdev: Add NULL get_tunnel_config for BSD.
          socket-util: restore building on FreeBSD.
          netdev-bsd: Use UINT64_MAX for unsupported stats.
          netdev-bsd: Adjust argument line wrapping
          configure: Add if_mib.h prerequisites.
          netdev-bsd: Correct pointer use after refactoring.
          netdev-bsd: Silence warnings on unimplemented platform.
          lib: Restore build on FreeBSD
          bfd: Include prerequisite header for FreeBSD
          tests: Fix build on FreeBSD
          util: Include pthread_np.h on FreeBSD

Thanks,

Ben.