> Date: Sat, 11 Sep 2021 17:37:32 -0700
> From: Jason Thorpe <thor...@me.com>
>
> > On Sep 11, 2021, at 4:29 PM, Taylor R Campbell
> > <campbell+netbsd-tech-k...@mumble.net> wrote:
> >
> > Here's another sketch that is much more flexible and general.
> > Adapting it to link sets should be easy; the main point is that the
> > only untyped parts going through void * are limited to the definition
> > site of a generic operation; everything else is fully typed.
>
> Looks quite verbose, but I'll study it some more. The sketch seems
> somewhat incomplete, so I'm not really getting how you're suggesting
> it should be used.
To define an operation (a `device call' or a `generic') that can be
implemented and invoked, create a .h file and a .c file for the
interface.
- The .h file contains
(a) a convenience typedef for the operation's signature,
(b) a macro for creating a vtable entry, and
(c) a wrapper for extracting the vtable entry into the correct type.
(The i2c_enumerate_devices_typecheck declaration in the example
isn't actually defined as a symbol -- it's just there to allow
compile-time type-checking of I2C_ENUMERATE_DEVICES_METHOD, so it
can detect when the function has the wrong type before converting to
void *.)
- The .c file contains
(a) a definition of a symbol so the linker guarantees it's unique
(link-time namespace collision detection),
(b) a definition of the wrapper function that extracts the vtable
entry with the correct type.
Nothing else needs to be in the .c file -- it's very small (a few
bytes) and is just needed by any code that uses the interface. The
wrapper function could also be in the .h file as a static inline.
A lot of this can easily be gathered into a handful of macros. E.g.,
i2c_bus_interface.h: DECLARE_GENERIC(int, i2c_enumerate_devices,
(struct i2c_attach_args *, bool (*) [...]));
#define I2C_ENUMERATE_DEVICES_METHOD(fn) \
METHOD(i2c_enumerate_devices, fn)
i2c_bus_interface.c: DEFINE_GENERIC(i2c_enumerate_devices);
A component that implements an operation just puts it in its vtable
(or link set or whatever):
#include "i2c_enumdevices.h"
static const struct vtable_entry acpi_device_handle_methods[] = {
I2C_ENUMERATE_DEVICES_METHOD(acpi_i2c_enumerate_devices),
...
};
static const struct vtable acpi_device_handle_vtable = {
acpi_device_handle_methods,
__arraycount(acpi_device_handle_methods),
};
(Adapting to a link set left as an exercise for the reader; the basic
mechanism remains the same -- a list of entries tagged by a defined
symbol, with a wrapper for the correct type of that defined symbol.)
A component that needs to use an operation pulls it out of the vtable
using the wrapper to use it:
struct i2c_attach_args ia;
int error;
error = i2c_enumerate_devices(h->dh_vtab)(&ia,
i2c_enumerate_devices_callback, cookie);
> Another thing I'm trying to comprehend here is just what kind of
> typos and type errors you think will be prevented. If the name of
> the `args' structure is wrong, it will either fail to compile
> (because the args structure name used doesn't exist, so accessing
> fields will fail to compile), or the error will be glaringly obvious
> because you used `spi_enumerate_devices_args' when you should have
> use `i2c_enumerate_devices_args' and the member names are different.
struct spi_enumerate_device_args args = ...;
/*
* runtime memory corruption -- simply operates on the wrong
* type of object, when it should be a trivially detected
* compile-time type error
*/
rv = device_call(h, "i2c-enumerate-devices", &args);
/*
* quiet loss of functionality -- simply returns ENOTSUP at
* runtime, when it should be a trivially detected
* compile-time typo error
*/
rv = device_call(h, "spi-enumerate-device", &args);
/*
* more runtime memory corruption -- I've been bitten by
* something like pcibus_attach_args vs pci_attach_args
* because autoconf is not type-safe
*/
static int
acpi_i2c_enumerate_devices(device_t dev, devhandle_t handle, void *v)
{
struct i2cbus_enumerate_devices_args *args = v;
... args->ia, args->callback ...
}
These are basic mistakes that at least I make all the time and that we
have had the technology to completely rule out at compile-time for
decades; I don't think it should be surprising for me to object if new
systems entirely under our control can't detect them.
> > If this doesn't meet the attributes you laid out earlier either, I
> > would appreciate it if you could identify which ones I missed --
> > there's too much going on here to fit in my head at once.
>
> [...]
> ==> Does not require referencing a symbol that would cause a link
> failure if a method is not available on some platform scheme.
> [...]
> But one problem your vtable sketch has is that is requires
> referencing a symbol (i2c_enumerate_devices_generic,
> i2c_enumerate_devices_typecheck, and i2c_enumerate_devices).
Why is this a requirement/problem?
The current mechanism still needs space in the kernel for the text of
the name (the string "i2c-enumerate-devices"). Why refuse to expose
that name to the linker so it can detect typos and namespace
collisions?
> I suppose I should adjust my #2 property above... in addition to not
> causing a link failure if e.g. ACPI doesn't provide some method, nor
> should there be a link failure if ACPI provides enumeration support
> for I2C but the I2C module isn't currently loaded.
Only a .c file for the interface -- not the whole i2c subsystem --
needs to be included.
