svn commit: r299108 - head/sys/sys

[Bruce Evans]

On Wed, 4 May 2016, John Baldwin wrote:

> On Thursday, May 05, 2016 02:51:31 AM Garrett Cooper wrote:
>>
>> Log:
>>   Revert r299096
>>
>>   The change broke buildworld when building lib/libkvm
>>
>>   This change likely needs to be run through a ports -exp run as a sanity
>>   check, as it might break downstream consumers.
>>
>>   Pointyhat to: adrian
>>   Reported by: kargl (confirmed on $work workstation)
>>   Sponsored by: EMC / Isilon Storage Division
>
> 'struct foo *' can be use with a simple forward declare in headers without
> requiring header pollution (and is often done for that reason).  device_t

style(9) also forbids using typedefs to obfuscate structs for that
reason.

style(9) doesn't even mention almost-correct use of opaque structs to hide
information, which bus.h actually does.  struct device is fully opaque
since it is only defined in subr_bus.c, so pointers to it cannot be
dereferenced elsewhere and are just cookies.  device_t is the type of
these pointers.  The information hiding from this is sort of negative.
The fact that device_t is a pointer to a struct must be exposed to use
device_t, by including the header that defines it.  The typedef just
makes it easier to change the implementation, but that is not needed
since a pointer to an opaque struct can be (ab)used to represent
anything and the only reasonable change is to a pointer to an opaque
union.  The typedef just makes the types (the opaque struct and
the typedefed type itself) harder to (mis)use.

In the original implementation, the typedef was private to <sys/bus.h>.
That made it very difficult to (mis)use the types.  You had to include
this header to use any bus API, and then you got the typedef automatically
so it was actually easier to use than 'struct device'.  This was broken in
r150521.  The typedef was moved to <sys/types.h> for the _KERNEL case and
was no longer defined in the !_KERNEL case if sys/bus.h is included in
that case.  sys/rman.h used to be careful about this.  In old versions
it exports a struct u_resource with an opaque r_device in it.  In
-current it exports a struct u_device with rather too much of the
externals of struct device exposed (in a translated form); this struct
is disgustingly formatted, with blind addition of the @brief ugliness
to every line of it breaking the length of every line.  It still uses
'struct device' instead of the typedef in its _KERNEL section to avoid
depending on <sys/bus.h>.

> should be used in any .c files, but headers might need to stick with
> 'struct device *' in a few cases for that reason.  I suspect both of these
> fall into that category.

Mostly in namespace-polluting cases.  This is the problem here.
sys/rman.h is careful not to export kernel pointers to userland, so
the changes there have no effect -- in the !_KERNEL case, device_t is
not used, and in the _KERNEL case the pollution in sys/types.h r150521
allows the typedef to work.  sys/pcpu.h is not so careful.  It declares
struct pcpu outside of its _KERNEL section, and this has the opaque
struct pointer and many other kernel pointers in it.

sys/rman.h and sys/pcpu.h are the only headers in <sys> that need to
declare the opaque struct [pointer] to avoid depending on other headers.
Moving the typedef to <sys/types.h> in r150521 seems to be just a bug.
The type was needed in 2 new functions, but since these functions are
in sys/bus.h, device_t was already visible in the correct way.

Bruce