Date: Thu, 5 May 2016 15:59:46 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@freebsd.org> Cc: Garrett Cooper <ngie@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r299108 - head/sys/sys Message-ID: <20160505150901.N1010@besplex.bde.org> In-Reply-To: <2598444.C6bcyDe9AO@ralph.baldwin.cx> References: <201605050251.u452pVSN034598@repo.freebsd.org> <2598444.C6bcyDe9AO@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160505150901.N1010>