Re: Introducing localcount(9)

[Chuck Silvers <chuq%chuq.com@localhost>]

On Tue, May 16, 2017 at 10:23:49PM +0000, Taylor R Campbell wrote:
> > Date: Tue, 16 May 2017 15:18:35 -0700
> > From: Chuck Silvers <chuq%chuq.com@localhost>
> > 
> > is the intent that in the long term everything should be converted to use
> > the new interfaces and that the non-"acquire" interfaces be removed?
> 
> That was what I had in mind.
> 
> > (that would be my preference, as well as keeping the original names...
> > the new names are long enough that they make it harder to read the code,
> > at least for me.)
> 
> Changing the name means code yet to be audited -- including code
> out-of-tree -- will fail to compile when we remove the old names.

if you think that is important, there are other ways to achieve it
which do not require abandoning good function names.
for example, you could require that drivers have

	#define DEVICE_INTERFACE_VERSION 2

before including <sys/device.h>, and if the value is less than "2"
than device.h could intentionally break the build with

	#define device_lookup break the build
	#define device_lookup_private break the build

for all the relevant functions.


> > if so, is the reason that you aren't converting everything now just that
> > you wanted to keep this change to a managable size?  I could help with
> > converting the rest of the drivers now if that would be useful.
> 
> Not just diffs but testing.

yes.  just like the all changes in the branch, right?


> > rather than require every driver to use the "DEVSW_MODULE_INIT" hack
> > when defining a devsw structure, it would be better to have bdevsw_attach()
> > and cdevsw_attach() take care of the localcount details without the
> > driver author needing to be aware of them.  I'd also think it would be
> > better to have the d_localcount field be the actual "struct localcount"
> > rather than a pointer to one, since you already enforce that the d_localcount
> > pointer is non-NULL and not shared between structures.
> 
> That's what I would have done originally, but embedding a mutable
> structure inside a `const struct cdevsw' doesn't work very well.

then let's just remove the "const".


> > device_acquire() seems of dubious value... it's only safe to use when one
> > already holds a reference on the device_t, so having it as an interface
> > is just inviting people to try to do fancy (and probably wrong) things
> > with the reference counting.  in your branch, you only use it in ld.c
> > (where it's either unnecessary or you're using it unsafely) and in fss.c
> > (which is just working around the fact that config_attach_pseudo()
> > returns a device_t with no reference, and it would seem better to fix
> > config_attach_pseudo() rather than work around that in the fss driver.)
> > I think it would be better to not have a device_acquire() interface
> > exposed to drivers.
> 
> Concur.


-Chuck