| > 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.

The typedef is crack in the abstraction layer rather than a specification
of type.

I want `opaque_t' to be large enough to hold any scalar type.  Come to
think about it, I suppose I should just bite the bullet and make it an
`int64_t'.

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

No, the value is the key, although the key can be the address of some
kernel datastructure.

| > 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.

PROP_CONST and PROP_AGGREGATE are type flags and encoded in the type
field.

| 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-)

We've already gone over this several times before.  Properties are a
convenient way for different parts of the kernel to communicate with
each other without having to resort to global variables.  It is not
designed for communication between different machines, therefore
self-describing datastructures are inappropriate.

| 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.

That's what the code does, although I don't think we really want to
bother with the signed/unsigned issue.  Better just to print it in hex.

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

What benefit do you get knowing the size of an aggregate in an array
if you don't know the types within that aggregate?

| array of strings would be disallowed.

Don't see why that should be the case.

| > 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-)

I am going to wait until there is some sort of consensus before making any
name changes.

| > 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.

Once again, I don't see why the database code should be setting policy.

| > 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...)

Objects do not exist.  Objects have relationships with properties.

| > 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-)

Fine.

| > 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?

Different databases are different data domains.  Would you ever copy between 
a proc and a vnode?

|
|
| > /* 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)...

Fine.

| why have 'valoff' rather than 'namelen'?  (i mean, valoff =
| namelen (incl nul) + offsetof kip_name, right?)
|
| name includes a NUL here, right?

Names are NUL terminated.  However, the code may take the liberty of aligning
the data, so the end of the string may not correspond directly with
the beginning of the value field.

|
|
| > 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-)

Existing documentation uses `quad' and `quad[]'.

|
| > 	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....)

Fine.

|
|
| > 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 

This follows the documentation for KERN_PROC sysctl()s.

|
| 	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-)

Things should work fine if `int' is 64 bits.  Lots of things will break
if it's 16 bits, so that's not really worth discussing.  However,
the notion of arbitrary width identifiers does cause problems with 
KERN_GETPROP, KERN_SETPROP, and KERN_DELPROP, where you have two 
separate 64-bit values that need to be encoded in the MIBs.  Determining
the beginning of the second value becomes an interesting problem.

| > 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-)

Yes.

|
|
| > 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?

In this case, no.  I consider that somewhat dangerous.

|
| 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?

I suppose we could have an in-kernel function to look up databases by
name if it's really necesary, but I don't think that will be the case.
Since each database should be for a specific information domain, if you
can't figure out how to get to the database, you probably shouldn't have
access to the information.

Eduardo