svn commit: r258142 - head/sys/net

[Bruce Evans]

On Thu, 14 Nov 2013, George Neville-Neil wrote:

> On Nov 14, 2013, at 16:03 , Sergey Kandaurov <pluknet at FreeBSD.org> wrote:
>
>> On 15 November 2013 00:07, George V. Neville-Neil <gnn at freebsd.org> wrote:
>>> Log:
>>>  Shift our OUI correctly.
>>>
>>>  Pointed out by: emaste
>>>
>>> Modified:
>>>  head/sys/net/ieee_oui.h
>>>
>>> Modified: head/sys/net/ieee_oui.h
>>> ==============================================================================
>>> --- head/sys/net/ieee_oui.h     Thu Nov 14 19:53:35 2013        (r258141)
>>> +++ head/sys/net/ieee_oui.h     Thu Nov 14 20:07:17 2013        (r258142)
>>> @@ -62,5 +62,5 @@
>>>  */
>>>
>>> /* Allocate 64K to bhyve */
>>> -#define OUI_FREEBSD_BHYVE_LOW  OUI_FREEBSD + 0x000001
>>> -#define OUI_FREEBSD_BHYVE_HIGH OUI_FREEBSD + 0x00ffff
>>> +#define OUI_FREEBSD_BHYVE_LOW  ((OUI_FREEBSD << 3) + 0x000001)
>>> +#define OUI_FREEBSD_BHYVE_HIGH ((OUI_FREEBSD << 3) + 0x00ffff)

This also fixes the syntax bugs (missing parentheses).

>> Shouldn't it be rather shifted with 24 (3 x 8bits)?
>
> Correct, and it should really be an | not a +.  I’ll commit a fix.

Any chance of also fixing the style bugs?  The most obvious ones are
spaces instead of tabs after #define's.

From the next commit:

% Modified: head/sys/net/ieee_oui.h
% ==============================================================================
% --- head/sys/net/ieee_oui.h	Thu Nov 14 21:31:58 2013	(r258146)
% +++ head/sys/net/ieee_oui.h	Thu Nov 14 21:57:37 2013	(r258147)
% ...
% @@ -62,5 +62,5 @@
%   */
% 
%  /* Allocate 64K to bhyve */
% -#define OUI_FREEBSD_BHYVE_LOW	((OUI_FREEBSD << 3) + 0x000001)
% -#define OUI_FREEBSD_BHYVE_HIGH	((OUI_FREEBSD << 3) + 0x00ffff)
% +#define OUI_FREEBSD_BHYVE_LOW	(((uint64_t)OUI_FREEBSD << 24) | 0x000001)
% +#define OUI_FREEBSD_BHYVE_HIGH	(((uint64_t)OUI_FREEBSD << 24) | 0x00ffff)
%

This introduces a new bug: uint64_t is used but isn't defined.  To avoid
all of the following design errors/style bugs:

- requiring applications to include <stdint.h> to use the BHYVE part of this
   header
- polluting this header for all applications by declaring uint64_t
   unconditionally in case the BHIVE parts are used
- same as previous, but without (user-visible) pollution by using __uint64_t
   instead of uint64_t
- breaking use of the definitions in cpp expressions by using any
   typedefed type in them
- using the long long abomination instead of uint64_t in the cast to
   avoid the pollution and make the definition possibly useable in cpp
   expressions, though it may still have the wrong type
- using the long long abomination as a type suffix for OUI_FREEBSD
   instead of casting OUI_FREEEBSD,

I suggest #defining OUI_FREEBSD as some literal constant shifted right
by 24.  Its type is then determined by the C rules for types of
literals combined with the rules for right shifts.  Shifting OUI_FREEBSD
back gives the same type as the original literal (this is not obvious)
the same value as the original literal, and the same value as the left
shift expressions above.  (Except in cpp expressions types don't work
normally.)

Does the API require defining OUI_FREEBSD as a value that needs to be
shifted left by 24 to use?  It would be more useful to define it as the
left-shifted value.  Then it gives the base for the FreeBSD range.
Values that aren't left shifted have the advantage of fitting in 32 bits,
so that their natural type is always int the type needed to hold the
left-shifted value can be implementation-defined (it should be int64_t
on 32-bit systems and long on 64-bit systems).

Bruce