Module Name: src
Committed By: thorpej
Date: Tue Aug 3 15:00:15 UTC 2021
Modified Files:
src/sys/kern [thorpej-cfargs2]: subr_autoconf.c
src/sys/sys [thorpej-cfargs2]: device.h
Log Message:
Address concerns about limited compile-time type checking with the
tag-value mechanism of specifying arguments to config_search(),
config_found(), and config_attach() by replacing the tag-value scheme
with a "struct cfargs", a pointer to which is passed to the aforementioned
functions instead.
The structure has a version field to allow for future ABI versioning
flexibility. The external structure is canononicalized internally
before use.
To ease the initialization of this structure, use a variadic preprocessor
macro, CFARGS(), to construct an anonymous "struct cfargs" inline, the
address of which is passed to the target function. A CFARGS_NONE macro
provides a symbolic stand-in for when the caller doesn't want to pass
arguments (currently expands to NULL and is handled during canonicalization).
To generate a diff of this commit:
cvs rdiff -u -r1.288 -r1.288.2.1 src/sys/kern/subr_autoconf.c
cvs rdiff -u -r1.171 -r1.171.2.1 src/sys/sys/device.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/kern/subr_autoconf.c
diff -u src/sys/kern/subr_autoconf.c:1.288 src/sys/kern/subr_autoconf.c:1.288.2.1
--- src/sys/kern/subr_autoconf.c:1.288 Mon Jun 14 08:55:49 2021
+++ src/sys/kern/subr_autoconf.c Tue Aug 3 15:00:15 2021
@@ -1,4 +1,4 @@
-/* $NetBSD: subr_autoconf.c,v 1.288 2021/06/14 08:55:49 skrll Exp $ */
+/* $NetBSD: subr_autoconf.c,v 1.288.2.1 2021/08/03 15:00:15 thorpej Exp $ */
/*
* Copyright (c) 1996, 2000 Christopher G. Demetriou
@@ -76,10 +76,8 @@
* @(#)subr_autoconf.c 8.3 (Berkeley) 5/17/94
*/
-#define __SUBR_AUTOCONF_PRIVATE /* see <sys/device.h> */
-
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: subr_autoconf.c,v 1.288 2021/06/14 08:55:49 skrll Exp $");
+__KERNEL_RCSID(0, "$NetBSD: subr_autoconf.c,v 1.288.2.1 2021/08/03 15:00:15 thorpej Exp $");
#ifdef _KERNEL_OPT
#include "opt_ddb.h"
@@ -168,6 +166,20 @@ struct alldevs_foray {
struct devicelist af_garbage;
};
+/*
+ * Internal version of the cfargs structure; all versions are
+ * canonicalized to this.
+ */
+struct cfargs_internal {
+ union {
+ cfsubmatch_t submatch;/* submatch function (direct config) */
+ cfsearch_t search; /* search function (indirect config) */
+ };
+ const char * iattr; /* interface attribute */
+ const int * locators; /* locators array */
+ devhandle_t devhandle; /* devhandle_t (by value) */
+};
+
static char *number(char *, int);
static void mapply(struct matchinfo *, cfdata_t);
static void config_devdelete(device_t);
@@ -177,6 +189,8 @@ static void config_devlink(device_t);
static void config_alldevs_enter(struct alldevs_foray *);
static void config_alldevs_exit(struct alldevs_foray *);
static void config_add_attrib_dict(device_t);
+static device_t config_attach_internal(device_t, cfdata_t, void *,
+ cfprint_t, const struct cfargs_internal *);
static void config_collect_garbage(struct devicelist *);
static void config_dump_garbage(struct devicelist *);
@@ -1068,65 +1082,37 @@ config_probe(device_t parent, cfdata_t c
return config_match(parent, cf, aux);
}
-static void
-config_get_cfargs(cfarg_t tag,
- cfsubmatch_t *fnp, /* output */
- const char **ifattrp, /* output */
- const int **locsp, /* output */
- devhandle_t *handlep, /* output */
- va_list ap)
-{
- cfsubmatch_t fn = NULL;
- const char *ifattr = NULL;
- const int *locs = NULL;
- devhandle_t handle;
-
- devhandle_invalidate(&handle);
-
- while (tag != CFARG_EOL) {
- switch (tag) {
- /*
- * CFARG_SUBMATCH and CFARG_SEARCH are synonyms, but this
- * is merely an implementation detail. They are distinct
- * from the caller's point of view.
- */
- case CFARG_SUBMATCH:
- case CFARG_SEARCH:
- /* Only allow one function to be specified. */
- if (fn != NULL) {
- panic("%s: caller specified both "
- "SUBMATCH and SEARCH", __func__);
- }
- fn = va_arg(ap, cfsubmatch_t);
- break;
-
- case CFARG_IATTR:
- ifattr = va_arg(ap, const char *);
- break;
+static struct cfargs_internal *
+cfargs_canonicalize(const struct cfargs * const cfargs,
+ struct cfargs_internal * const store)
+{
+ struct cfargs_internal *args = store;
- case CFARG_LOCATORS:
- locs = va_arg(ap, const int *);
- break;
+ memset(args, 0, sizeof(*args));
- case CFARG_DEVHANDLE:
- handle = va_arg(ap, devhandle_t);
- break;
+ /* If none specified, are all-NULL pointers are good. */
+ if (cfargs == NULL) {
+ return args;
+ }
- default:
- panic("%s: unknown cfarg tag: %d\n",
- __func__, tag);
- }
- tag = va_arg(ap, cfarg_t);
+ /*
+ * submatch and search are mutually-exclusive.
+ */
+ if (cfargs->submatch != NULL && cfargs->search != NULL) {
+ panic("cfargs_canonicalize: submatch and search are "
+ "mutually-exclusive");
}
+ if (cfargs->submatch != NULL) {
+ args->submatch = cfargs->submatch;
+ } else if (cfargs->search != NULL) {
+ args->search = cfargs->search;
+ }
+
+ args->iattr = cfargs->iattr;
+ args->locators = cfargs->locators;
+ args->devhandle = cfargs->devhandle;
- if (fnp != NULL)
- *fnp = fn;
- if (ifattrp != NULL)
- *ifattrp = ifattr;
- if (locsp != NULL)
- *locsp = locs;
- if (handlep != NULL)
- *handlep = handle;
+ return args;
}
/*
@@ -1140,25 +1126,23 @@ config_get_cfargs(cfarg_t tag,
* an arbitrary function to all potential children (its return value
* can be ignored).
*/
-cfdata_t
-config_vsearch(device_t parent, void *aux, cfarg_t tag, va_list ap)
+static cfdata_t
+config_search_internal(device_t parent, void *aux,
+ const struct cfargs_internal * const args)
{
- cfsubmatch_t fn;
- const char *ifattr;
- const int *locs;
struct cftable *ct;
cfdata_t cf;
struct matchinfo m;
- config_get_cfargs(tag, &fn, &ifattr, &locs, NULL, ap);
-
KASSERT(config_initialized);
- KASSERT(!ifattr || cfdriver_get_iattr(parent->dv_cfdriver, ifattr));
- KASSERT(ifattr || cfdriver_iattr_count(parent->dv_cfdriver) < 2);
+ KASSERT(!args->iattr ||
+ cfdriver_get_iattr(parent->dv_cfdriver, args->iattr));
+ KASSERT(args->iattr ||
+ cfdriver_iattr_count(parent->dv_cfdriver) < 2);
- m.fn = fn;
+ m.fn = args->submatch; /* N.B. union */
m.parent = parent;
- m.locs = locs;
+ m.locs = args->locators;
m.aux = aux;
m.match = NULL;
m.pri = 0;
@@ -1186,7 +1170,8 @@ config_vsearch(device_t parent, void *au
* consider only children which attach to
* that attribute.
*/
- if (ifattr && !STREQ(ifattr, cfdata_ifattr(cf)))
+ if (args->iattr != NULL &&
+ !STREQ(args->iattr, cfdata_ifattr(cf)))
continue;
if (cfparent_match(parent, cf->cf_pspec))
@@ -1197,14 +1182,13 @@ config_vsearch(device_t parent, void *au
}
cfdata_t
-config_search(device_t parent, void *aux, cfarg_t tag, ...)
+config_search(device_t parent, void *aux, const struct cfargs *cfargs)
{
cfdata_t cf;
- va_list ap;
+ struct cfargs_internal store;
- va_start(ap, tag);
- cf = config_vsearch(parent, aux, tag, ap);
- va_end(ap);
+ cf = config_search_internal(parent, aux,
+ cfargs_canonicalize(cfargs, &store));
return cf;
}
@@ -1256,18 +1240,17 @@ static const char * const msgs[] = {
* not configured, call the given `print' function and return NULL.
*/
device_t
-config_vfound(device_t parent, void *aux, cfprint_t print, cfarg_t tag,
- va_list ap)
+config_found(device_t parent, void *aux, cfprint_t print,
+ const struct cfargs * const cfargs)
{
cfdata_t cf;
- va_list nap;
-
- va_copy(nap, ap);
- cf = config_vsearch(parent, aux, tag, nap);
- va_end(nap);
+ struct cfargs_internal store;
+ const struct cfargs_internal * const args =
+ cfargs_canonicalize(cfargs, &store);
+ cf = config_search_internal(parent, aux, args);
if (cf != NULL) {
- return config_vattach(parent, cf, aux, print, tag, ap);
+ return config_attach_internal(parent, cf, aux, print, args);
}
if (print) {
@@ -1292,19 +1275,6 @@ config_vfound(device_t parent, void *aux
return NULL;
}
-device_t
-config_found(device_t parent, void *aux, cfprint_t print, cfarg_t tag, ...)
-{
- device_t dev;
- va_list ap;
-
- va_start(ap, tag);
- dev = config_vfound(parent, aux, print, tag, ap);
- va_end(ap);
-
- return dev;
-}
-
/*
* As above, but for root devices.
*/
@@ -1316,7 +1286,7 @@ config_rootfound(const char *rootname, v
KERNEL_LOCK(1, NULL);
if ((cf = config_rootsearch(NULL, rootname, aux)) != NULL)
- dev = config_attach(ROOT, cf, aux, NULL, CFARG_EOL);
+ dev = config_attach(ROOT, cf, aux, NULL, CFARGS_NONE);
else
aprint_error("root device %s not configured\n", rootname);
KERNEL_UNLOCK_ONE(NULL);
@@ -1539,8 +1509,8 @@ config_unit_alloc(device_t dev, cfdriver
}
static device_t
-config_vdevalloc(const device_t parent, const cfdata_t cf, cfarg_t tag,
- va_list ap)
+config_devalloc(const device_t parent, const cfdata_t cf,
+ const struct cfargs_internal * const args)
{
cfdriver_t cd;
cfattach_t ca;
@@ -1552,7 +1522,6 @@ config_vdevalloc(const device_t parent,
void *dev_private;
const struct cfiattrdata *ia;
device_lock_t dvl;
- const int *locs;
cd = config_cfdriver_lookup(cf->cf_name);
if (cd == NULL)
@@ -1571,12 +1540,7 @@ config_vdevalloc(const device_t parent,
}
dev = kmem_zalloc(sizeof(*dev), KM_SLEEP);
- /*
- * If a handle was supplied to config_attach(), we'll get it
- * assigned automatically here. If not, then we'll get the
- * default invalid handle.
- */
- config_get_cfargs(tag, NULL, NULL, &locs, &dev->dv_handle, ap);
+ dev->dv_handle = args->devhandle;
dev->dv_class = cd->cd_class;
dev->dv_cfdata = cf;
@@ -1598,7 +1562,7 @@ config_vdevalloc(const device_t parent,
xunit = number(&num[sizeof(num)], myunit);
lunit = &num[sizeof(num)] - xunit;
if (lname + lunit > sizeof(dev->dv_xname))
- panic("config_vdevalloc: device name too long");
+ panic("config_devalloc: device name too long");
dvl = device_getlock(dev);
@@ -1613,13 +1577,14 @@ config_vdevalloc(const device_t parent,
else
dev->dv_depth = 0;
dev->dv_flags |= DVF_ACTIVE; /* always initially active */
- if (locs) {
+ if (args->locators) {
KASSERT(parent); /* no locators at root */
ia = cfiattr_lookup(cfdata_ifattr(cf), parent->dv_cfdriver);
dev->dv_locators =
kmem_alloc(sizeof(int) * (ia->ci_loclen + 1), KM_SLEEP);
*dev->dv_locators++ = sizeof(int) * (ia->ci_loclen + 1);
- memcpy(dev->dv_locators, locs, sizeof(int) * ia->ci_loclen);
+ memcpy(dev->dv_locators, args->locators,
+ sizeof(int) * ia->ci_loclen);
}
dev->dv_properties = prop_dictionary_create();
KASSERT(dev->dv_properties != NULL);
@@ -1639,19 +1604,6 @@ config_vdevalloc(const device_t parent,
return dev;
}
-static device_t
-config_devalloc(const device_t parent, const cfdata_t cf, cfarg_t tag, ...)
-{
- device_t dev;
- va_list ap;
-
- va_start(ap, tag);
- dev = config_vdevalloc(parent, cf, tag, ap);
- va_end(ap);
-
- return dev;
-}
-
/*
* Create an array of device attach attributes and add it
* to the device's dv_properties dictionary.
@@ -1734,9 +1686,9 @@ config_add_attrib_dict(device_t dev)
/*
* Attach a found device.
*/
-device_t
-config_vattach(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
- cfarg_t tag, va_list ap)
+static device_t
+config_attach_internal(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
+ const struct cfargs_internal * const args)
{
device_t dev;
struct cftable *ct;
@@ -1745,7 +1697,7 @@ config_vattach(device_t parent, cfdata_t
KASSERT(KERNEL_LOCKED_P());
- dev = config_vdevalloc(parent, cf, tag, ap);
+ dev = config_devalloc(parent, cf, args);
if (!dev)
panic("config_attach: allocation of device softc failed");
@@ -1817,18 +1769,14 @@ config_vattach(device_t parent, cfdata_t
device_t
config_attach(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
- cfarg_t tag, ...)
+ const struct cfargs *cfargs)
{
- device_t dev;
- va_list ap;
+ struct cfargs_internal store;
KASSERT(KERNEL_LOCKED_P());
- va_start(ap, tag);
- dev = config_vattach(parent, cf, aux, print, tag, ap);
- va_end(ap);
-
- return dev;
+ return config_attach_internal(parent, cf, aux, print,
+ cfargs_canonicalize(cfargs, &store));
}
/*
@@ -1847,7 +1795,8 @@ config_attach_pseudo(cfdata_t cf)
KERNEL_LOCK(1, NULL);
- dev = config_devalloc(ROOT, cf, CFARG_EOL);
+ struct cfargs_internal args = { };
+ dev = config_devalloc(ROOT, cf, &args);
if (!dev)
goto out;
Index: src/sys/sys/device.h
diff -u src/sys/sys/device.h:1.171 src/sys/sys/device.h:1.171.2.1
--- src/sys/sys/device.h:1.171 Sat Jun 12 12:13:51 2021
+++ src/sys/sys/device.h Tue Aug 3 15:00:15 2021
@@ -1,4 +1,4 @@
-/* $NetBSD: device.h,v 1.171 2021/06/12 12:13:51 riastradh Exp $ */
+/* $NetBSD: device.h,v 1.171.2.1 2021/08/03 15:00:15 thorpej Exp $ */
/*
* Copyright (c) 2021 The NetBSD Foundation, Inc.
@@ -412,6 +412,7 @@ TAILQ_HEAD(cftablelist, cftable);
#endif
typedef int (*cfsubmatch_t)(device_t, cfdata_t, const int *, void *);
+typedef int (*cfsearch_t)(device_t, cfdata_t, const int *, void *);
/*
* `configuration' attachment and driver (what the machine-independent
@@ -530,19 +531,39 @@ struct pdevinit {
#define DVUNIT_ANY -1
/*
- * Tags for tag-value argument pairs passed to config_search() and
- * config_found().
+ * Arguments passed to config_search() and config_found().
*/
-typedef enum {
- CFARG_SUBMATCH = 0, /* submatch function (direct config) */
- CFARG_SEARCH = 1, /* search function (indirect config) */
- CFARG_IATTR = 2, /* interface attribute */
- CFARG_LOCATORS = 3, /* locators array */
- CFARG_DEVHANDLE = 4, /* devhandle_t (by value) */
-
- /* ...a value not likely to occur randomly in the wild. */
- CFARG_EOL = 0xeeeeeeee
-} cfarg_t;
+struct cfargs {
+ uintptr_t cfargs_version; /* version field */
+
+ /* version 1 fields */
+ cfsubmatch_t submatch; /* submatch function (direct config) */
+ cfsearch_t search; /* search function (indirect config) */
+ const char * iattr; /* interface attribute */
+ const int * locators; /* locators array */
+ devhandle_t devhandle; /* devhandle_t (by value) */
+
+ /* version 2 fields below here */
+};
+
+#define CFARGS_VERSION 1 /* current cfargs version */
+
+#define CFARGS_NONE NULL /* no cfargs to pass */
+
+/*
+ * Construct a cfargs with this macro, like so:
+ *
+ * CFARGS(.submatch = config_stdsubmatch,
+ * .devhandle = my_devhandle)
+ *
+ * You must supply at least one field. If you don't need any, use the
+ * CFARGS_NONE macro.
+ */
+#define CFARGS(...) \
+ &((const struct cfargs){ \
+ .cfargs_version = CFARGS_VERSION, \
+ __VA_ARGS__ \
+ })
#ifdef _KERNEL
@@ -587,25 +608,15 @@ const struct cfiattrdata *cfiattr_lookup
const char *cfdata_ifattr(const struct cfdata *);
int config_stdsubmatch(device_t, cfdata_t, const int *, void *);
-cfdata_t config_search(device_t, void *, cfarg_t, ...);
+cfdata_t config_search(device_t, void *, const struct cfargs *);
cfdata_t config_rootsearch(cfsubmatch_t, const char *, void *);
-device_t config_found(device_t, void *, cfprint_t, cfarg_t, ...);
+device_t config_found(device_t, void *, cfprint_t, const struct cfargs *);
device_t config_rootfound(const char *, void *);
-device_t config_attach(device_t, cfdata_t, void *, cfprint_t, cfarg_t, ...);
+device_t config_attach(device_t, cfdata_t, void *, cfprint_t,
+ const struct cfargs *);
int config_match(device_t, cfdata_t, void *);
int config_probe(device_t, cfdata_t, void *);
-#if defined(__SUBR_AUTOCONF_PRIVATE)
-/*
- * XXX Some ports abuse the internals of autoconfiguration, so we need
- * XXX provide these symbols to them for the time being.
- */
-cfdata_t config_vsearch(device_t, void *, cfarg_t, va_list);
-device_t config_vfound(device_t, void *, cfprint_t, cfarg_t, va_list);
-device_t config_vattach(device_t, cfdata_t, void *, cfprint_t, cfarg_t,
- va_list);
-#endif /* __SUBR_AUTOCONF_PRIVATE */
-
bool ifattr_match(const char *, const char *);
device_t config_attach_pseudo(cfdata_t);
