Re: [PATCH RFC 02/20] ctlv: Add the UAPI wire format and family API

[Jason Gunthorpe]

> [ ... 159 lines skipped ... ]
> +#define CTLV_NODE_SCHEMA_UNLIMITED 0
> +
> +#define __CTLV_NODE_SCHEMA(_attr, _kind, _occurrences, ...)	\
> +	{ .attr_id = (_attr), .kind = (_kind),			\
> +	  .max_occurrences = (_occurrences), __VA_ARGS__ }

This reads alot better without the extra macro layer. The point of
this is to allow the codegen output to be concise it is worth being a
bit more verbose here so the exact form is easier to see.

> +
> +#define __CTLV_NODE_SCHEMA_CHILDREN(_first, _count, _required)	\
> +	.first_child = (_first), .children_count = (_count),	\
> +	.required_children_count = (_required)
> +
> +#define CTLV_NODE_SCHEMA_ROOT(_first, _count, _required)	\
> +	__CTLV_NODE_SCHEMA(CTLV_ATTR_ROOT, CTLV_SCHEMA_NEST, 1,	\
> +			   __CTLV_NODE_SCHEMA_CHILDREN(_first, _count, _required))

For instance this is much more directly readable:

#define CTLV_NODE_SCHEMA_ROOT(_first, _count, _required) \
	((struct ctlv_node_schema){                      \
		.attr_id = CTLV_ATTR_ROOT,               \
		.kind = CTLV_SCHEMA_NEST,                \
		.max_occurrences = 1,                    \
		.first_child = _first,                   \
		.children_count = _count,                \
		.required_children_count = _required,    \
	})

Even if it is longer, and and clangd can now jump to the struct members

I also think each of these should have a one line comment explaining
when it is used.

> [ ... 61 lines skipped ... ]
> +#define __CTLV_STUB_SCHEMA(_family, _set, _node, _optional)	\
> +	{ .family_id = (_family), .set_id = (_set),		\
> +	  .node = (_node), .optional = (_optional) }

Same here

> +
> +#define CTLV_STUB_SCHEMA(_family, _set, _node)			\
> +	__CTLV_STUB_SCHEMA(_family, _set, _node, false)
> +
> +#define CTLV_STUB_SCHEMA_OPTIONAL(_family, _set, _node)		\
> +	__CTLV_STUB_SCHEMA(_family, _set, _node, true)
> +
> +struct ctlv_schema_tree {
> +	const struct ctlv_node_schema *node_schemas;
> +	u32 node_schemas_count;
> +	const struct ctlv_stub_schema *stub_schemas;
> +	u32 stub_schemas_count;
> +};

I've been ponding for a while what 'stub' means in this context.

AI suggested "extension_points would be a clearer name here." I guess
because ctlv.rst calls it "Inheritance and extension points" ?

> +
> +#define CTLV_SCHEMA_TREE(_name) {				\
> +	.node_schemas = _name##_node_schemas,			\
> +	.node_schemas_count = ARRAY_SIZE(_name##_node_schemas),	\
> +}

It is helpful to put the struct on these macros ((struct X){})

#define CTLV_SCHEMA_TREE(_name)                                         \
	((struct ctlv_schema_tree){                                     \
		.node_schemas = _name##_node_schemas,                   \
		.node_schemas_count = ARRAY_SIZE(_name##_node_schemas), \
	})

> [ ... 127 lines skipped ... ]
> +typedef size_t (*ctlv_family_priv_size_fn)(void);
> +typedef const void *(*ctlv_family_pre_init_fn)(const void *init_info);
> +typedef int (*ctlv_family_init_fn)(struct ctlv_device *ctlvdev,
> +				   const void *init_info, void *priv);
> +typedef int (*ctlv_family_post_init_fn)(struct ctlv_device *ctlvdev,
> +					void *priv);
> +typedef void (*ctlv_family_pre_fini_fn)(struct ctlv_device *ctlvdev,
> +					void *priv);
> +typedef void (*ctlv_family_fini_fn)(struct ctlv_device *ctlvdev, void *priv);
> +
> +typedef int (*ctlv_stub_fill_fn)(void *priv, struct ctlv_op_ctx *ctx,
> +				 const struct ctlv_cursor *request,
> +				 struct ctlv_cursor *nest);
> +
> +struct ctlv_stub_ops {
> +	u16 family_id;
> +	u32 set_id;
> +	ctlv_stub_fill_fn fill;
> +};
> +
> +#define CTLV_STUB_OPS(_family, _set, _name) {			\
> +	.family_id = (_family),					\
> +	.set_id = (_set),					\
> +	.fill = _name##_fill,					\
> +}
> +
> +struct ctlv_family {
> +	const struct ctlv_family_schema *schema;
> +	const struct ctlv_family *parent;
> +	const struct ctlv_op *ops;
> +	u32 ops_count;
> +	ctlv_op_pre_fn pre;
> +	ctlv_op_post_fn post;
> +	ctlv_family_priv_size_fn priv_size;
> +	ctlv_family_pre_init_fn pre_init;
> +	ctlv_family_init_fn init;
> +	ctlv_family_post_init_fn post_init;
> +	ctlv_family_pre_fini_fn pre_fini;
> +	ctlv_family_fini_fn fini;
> +	const struct ctlv_stub_ops *stub_ops;
> +	u32 stub_ops_count;

Having the function ops in a struct use typedefs instead of just
coding the signature is unkernelly, I couldn't figure out what this
was done, it seems there is no user of the typedef

> [ ... 182 lines skipped ... ]
> +/* Implementation limits, not wire-field maxima: raising them is compatible. */
> +
> +/* Request roots, action replies, query reply fragments and event roots. */
> +#define CTLV_MAX_INLINE_MESSAGE_LEN	0x100000	/* 1 MiB */
> +/* Includes the error information root. */
> +#define CTLV_MAX_ERROR_LEN		0x4000		/* 16 KiB */
> +/* The root attr is at depth zero. */
> +#define CTLV_MAX_NEST_DEPTH		32
> +#define CTLV_DEFAULT_QUERY_REPLY_LEN	0x10000		/* 64 KiB */
> +#define CTLV_MAX_EVENT_QUEUE_LEN	0x1000000	/* 16 MiB */

If they are just random kernel limits I'm not sure they should be in
this uapi header?

-- 
Jason