eeh@netbsd.org writes:
> The latest version of Generic Properties, which I expect to check in
> shortly is at:
> 
> 	ftp.netbsd.org:/pub/NetBSD/misc/eeh/generic_properties

So, I just looked at the text documentation file in there, with a
brief look at the header.
I'm assuming that the text file is meant to document the
implementation, and will turn into a section 9 manual page.

As you might have guessed, I'd not really had a chance to look at this
previously, but didn't know if others had looked and commented, and
therefore figured that i should before you accidentally mistake
silence for assent.  8-)

Anyway, comments below.  I don't know how many people have looked this
over, but based on some of the comments i have i suspect "not enough."
(some might say that it'd be nice to post this to the BSD API list run
by ... uh, your employer, too.  but i don't think i will.  8-)

Overall, I actually object to this a lot less than previous things
i've seen...  probably mostly because all of the searching crud is
gone.  Note that i've endeavored to _avoid_ looking at the .c file.
somebody using the interface shouldn't have to.  8-)


> 2.0	Public Data Structures
> 
> Two new opaque types are defined:
> 
> typedef void *kdatabase_t;	-- Database container object.

see below on the name of this.

> typedef void *opaque_t;		-- Container for opaque identifier.

from the description in the header of opaque_t:

typedef void *opaque_t;         /* Value large enough to hold a pointer */

s/Value/Type/  8-)

There might be some use in having this be a (u)intptr_t, if all you
want is a space big enough to hold a pointer.

Typically I imagine code will want to use a struct ptr, though... but
if you actually name the arg as "opaque_t" then it's probably
stylistically "better" to cast things to opaque_t when passing (even
though it's not necessary).  If you leave it as a typedef to a void *,
people won't do the cast but the purpose of making it a type (ability
to change later) is not really had...

(also, not sure, but does lint do better typechecking on stuff like
this to warn of bad type usage?)

So:

	* Why do you define this as a typedef, and

	* if you really want it as a typedef, you probably want it to
	be defined in terms of another type, IMO.

(you don't mean that the opaque_t value passed in to the function is
actually a _pointer_ to the key, do you?!)


> 2.1	Property Types
> 
> PROP_STRING			-- Property is a string.
> PROP_INT			-- Property is an integer value.
> PROP_ARRAY			-- Property is an array of the 
> 				   same type of values.
> PROP_AGREGATE			-- Property is an agregation of 
> 				   different types of values.

btw, "aggregate" and "aggregation."  (*sigh*)

> PROP_CONST			-- Property is constant, do not 
> 				   allocate storage for the data.

It's not stated here, but PROP_CONST is a flag.

OK, so you know something is an array, but how do you know how big the
array elements are?  (a priori knowledge, a.k.a. magic, bad. 8-)

I would suggest something like encoding:

	* string
	* int
	* unsigned int (the distinction nice, for userland printer)
	* aggregate

as your basic types, then have 'array' be a flag.

provide a way to encode int/aggregate size in the 'type' word, and
require it for arrays (size of the integers or aggregate types).

array of strings would be disallowed.


> 2.3	Functions
> 
> 
>    kdatabase_t createpropdb(const char *name);
>    void delpropdb(kdatabase_t db);
>    int setprop(kdatabase_t db, opaque_t object, const char *name, 
> 		void *val, size_t len, int type, int wait);
>    size_t listobjs(kdatabase_t db, opaque_t *objects, size_t len);
>    size_t listprops(kdatabase_t db, opaque_t object, char *names, 
> 		size_t len);
>    size_t getprop(kdatabase_t db, opaque_t object, const char *name,
> 		void *val, size_t len, int *type);
>    int delprop(kdatabase_t db, opaque_t object, const char *name);
>    int copyprops(kdatabase_t db, opaque_t source, opaque_t dest, 
> 			int wait);

This is really disappointing.  One thing that I think a couple of us
tried to stress _over and over_ in the device properties discussion
was that uniformity of function naming is fairly useful and important.

I suggest:

	kprop_createdb
	kprop_destroydb
	kprop_set
	kprop_listobjs
	kprop_listprops
	kprop_get
	kprop_delete
	kprop_copy

or _something_ other than the all-over-the-map names you have now.

As for the name of the "kdatabase_t" typedef, something that makes it
obvious that this is kprop-related would be good.  e.g. "kpropdb_t".  8-)


> kdatabase_t createpropdb(const char *name);
> 
> Allocate and initialize a kernel database object, and associate `name' with
> the database.  `name' may later be used to access this database from userland
> throught the userland database query interface.  This operation may block.
> Returns NULL on failure.

is 'name' duplicated, assumed to be "static" (allocated by caller or in
text) or what?  (I'd guess duplicated as part of the database
structure, but i didn't look at the C code and, well, it'd be good to
say what the fucntion requires re: the lifetime of its input args.  8-)


I'm thinking that maybe 'setprop' should reject an attempt to change
an existing property's type.  not sure tho.


> size_t listobjs(kdatabase_t db, opaque_t *objects, size_t len);
> 
> Get a list of objects from a database.  A list of objects will be copied
> into the buffer pointed to by `objects' up to `len' bytes.  It returns
> the amount of memory needed to store the entire list.

am i correct in thinking that an object exists iff there are
properties associated with it?

(that nature should be mentioned someplace...)


> size_t getprop(kdatabase_t db, opaque_t object, const char *name, 
> 	void *val, size_t len, int *type);
> 
> Retrieve a property called `name' associated with `object'.  Name is a pointer
> to a string.  The property that matches both `object' and `name' will be
> selected, and the data and type associated with that property will be returned
> in the buffers pointed to by `val' and `type' as appropriate.
> 
> Returns -1 if the property cannot be found, otherwise it returns the length of
> the value data and if `val' is not NULL it copies up to `len' bytes of the
> property data to the location pointed to by `val'.  The type value associated
> with that property is stored in the location pointed to by `type' if it is not
> NULL.

uh, why do the 'val != NULL' check if anything other than diagnostic?

I'd think the code should go something like:

#ifdef DIAGNOSTIC
	assert val != NULL || len == 0
#endif
	memcpy(val, actual_contents, min(len, content_len));
	return (content_len);

yeah, it's a bloody small optimization, but hey, if you're going to
define an interface why define it so that it _must_ do something that
otherwise it wouldn't have to do when that is redundant.  8-)
	

> int delprop(kdatabase_t db, opaque_t object, const char *name);
> 
> Remove a property from a database.  If a NULL is supplied for the name,
> dev_delprop(9) will remove all properties associated with `object'.

(and, therefore effectively deletes the 'object' entirely from the
database?)


> int copyprops(kdatabase_t db, opaque_t source, opaque_t dest, 
> 		int wait);
> 
> Copy all properties associated with `source' to `dest' structure.  If `wait'
> is zero then dev_copyprops(9) will not sleep for resource shortage.  Returns 0
> on success or an error value.  The state of properties is undefined if the
> operation fails.

Would you ever want to copy _between_ two databases?


> /* A single property */
> struct kinfo_prop {
> 	int	kip_len;		/* total len of this prop */
> 	int	kip_type;		/* type of this prop */
> 	int	kip_valoff;		/* offset of start of value */
> 	int	kip_vallen;		/* length of value */
> 	char	kip_name[1];		/* name of this property */
> };

Shouldn't all of these be fixed-sized types rather than ints (probably
uint32_t for all)...

why have 'valoff' rather than 'namelen'?  (i mean, valoff =
namelen (incl nul) + offsetof kip_name, right?)

name includes a NUL here, right?


> 4.2	Sysctl MIBs

I'm a bit puzzled by some of these bits...


> A new KERN_DB MIB is provides the following:
> 
> 	Third level name	Type			Changeable
> 	----------------	------			----------
> 	KERN_DB_ALL		struct kinfo_kdb[]	no
> 	KERN_OBJ_ALL		quad[]			no

shouldn't that be uint64_t? or int64_t?  8-)

> 	KERN_PROP_ALL		struct kinfo_prop[]	no
> 	KERN_GETPROP		struct kinfo_prop	no
> 	KERN_SETPROP		struct kinfo_prop	yes
> 	KERN_DELPROP		struct kinfo_prop	yes

Shouldn't these be KERN_PROPDB_XXX? or something?

(i'm concerned about them looking like they're at the 'kern.foo' level
from the names, rather than the 'kern.propdb.foo' level....)


> KERN_DB_ALL
> 
> Returns an array of kinfo_kdb structures describing all existing in-kernel
> databases.  The ki_id field is the identifier to that should be provided when
> using sysctl to query that database.
> 
> 
> KERN_OBJ_ALL
> 
> The fourth and fifth-level MIBs should be the kernel database identifier for a
> particular database that was provided in the ki_id field from an earlier
> KERN_DB_ALL query.  It returns an array of 64-bit object identifiers.

"Fourth and fifth" starting from 0 or 1 (based on quick check of
sysctl documentation, i'd guess 1, since it looks like CTL_KERN = 0,
KERN_DB = 1, KERN_OBJ_ALL 

	uint64_t x = objid;
	int mib;

	mib[0] = CTL_KERN;
	mib[1] = KERN_DB;
	mib[2] = (better name for KERN_OBJ_ALL);
	memcpy(&mib[3], x, sizeof x);

In that case, wouldn't it potentially be "fourth and susequent" or
something?  (what if 'int' is 64 bits, or 16? 8-) yeah, a lot of code
breaks, but... 8-)


> KERN_PROP_ALL
> 
> The fourth and fifth-level MIBs should be the kernel database identifier, and
> the sixth and seventh-level MIBs should be the object identifier.  This
> returns an array of kinfo_prop structures for each property associated with
> that database identifier and object identifier.

same question here.



> KERN_SETPROP
> 
> The fourth and fifth-level MIBs should be the kernel database identifier, and
> the sixth and seventh-level MIBs should be the object identifier.  A
> kinfo_prop structure should be completely filled out.  The old property
> associated with the specified database and object with the specified name is
> returned, and is replaced with the property provided.

error to specify const-ness, eh?  8-)


> KERN_DELPROP
> 
> The fourth and fifth-level MIBs should be the kernel database identifier, and
> the sixth and seventh-level MIBs should be the object identifier.  A
> kinfo_prop structure should be filled out with valid kip_len and kip_name
> fields.  The property associated with the specified database and object with
> the specified name is returned and removed from the database.

special value (#defined name for 0) that causes all properties to be
delete, as in the fn call?


looking at the fn names and the sysctl mib names, i think there should
be some correspondence.

e.g. if it's

        kprop_*

then it should probably be:

	kern.prop.*

but maybe: kpropdb_* -> kern.propdb.* etc.  the last-level names and
the corresponding function calls should probably match up to some
degree, too.

actually, that makes me wonder:

in-kernel function to come up with the list of property DBs, for
orthogonality?  or just punt (probably make it static) since it's
probably only going to be used inside the module?


chris