sol-flow-static shouldn't always duplicate options. #335
Description
sol-flow-static.c:flow_node_open() is always calling sol_flow_node_get_options() for each children options, this effectively allocates memory and copies the options, then need to call sol_flow_node_free_options() right after that -- already bad per se.
However, if the type doesn't specify new_options() callback, an empty_defaults is used. This is bad, since the original options was available and were not used.
I got to this when creating a type manually and not specifying the new_options() callback, as it's not required for my use case (simplectype -- to be sent as PR soon).
I'm not changing this as it's not clear to me why options needed to be duplicated at all. Even in the case of a builder, where you create new options from a textual description, those should be stored in the builder-generated type itself when such type is created.
I also see but do not understand the purpose of child_opts_set() callback that is called per-instance of flow, but stores the information per-type (class). It looks wrong, it should be pushing that information to node and should only be used if we're getting an option on a parent flow/node that should be redirected to a children node -- not my case, where I have no exported options in my parent.
To support exporting options, that will result in options being sent/propagated to children, I believe that the most efficient and clear way is making exported options a separate vector we have as options for static flows (ie: struct sol_flow_static_node_options) that we iterate in a way similar to what we do for ports:
sol-flow-static.h defines
struct sol_flow_static_node_options {
const struct sol_flow_node_options base;
#define SOL_FLOW_STATIC_NODE_OPTIONS_API_VERSION 1
const struct sol_flow_node_options {
uint16_t node;
const struct sol_flow_node_options *opts;
} children_options;
};
struct sol_flow_static_option_spec {
uint16_t node;
/* some stuff to define how to export, like members or the whole struct
* using whole struct here, so only taking the node index.
* we could take the member index within the options description.
*/
};sol-flow-static.c uses
struct sol_flow_node_options *
get_exported_options(const struct flow_static_type *type, uint16_t node_idx, struct sol_flow_static_node_options *flow_opts) {
if (type->exported_options && flow_opts && flow_opts->children_options) {
uint16_t i;
/* ensure node option is exported */
for (i = 0; type->exported_options[i]->node != UINT16_MAX; i++) {
if (type->exported_options[i]->node == node_idx) {
uint16_t j;
for (j = 0; flow_opts->children_options[j].node != UINT16_MAX; i++) {
if (flow_opts->children_options[j].node == node_idx)
return flow_opts->childre_options[j].opts;
}
}
}
return NULL;
}
flow_node_open(...) {
for (spec = type->node_specs, i = 0; spec->type != NULL; spec++, i++) {
struct sol_flow_node *child_node = fsd->nodes[i];
const struct sol_flow_node_options *def_child_opts = spec->opts;
const struct sol_flow_node_options *exported_child_opts = get_exported_options(type, i, flow_opts);
const struct sol_flow_node_options *child_opts = exported_child_opts || def_child_opts;
/* this way, if exported_child_opts is something we allocate, we could free it later
* by checking exported_child_opts.
* it would be the case if we want to export not the full set of options from a child node,
* but only parts of it. Like exposing the GPIO pin, but not the edge_mode.
*/
r = sol_flow_node_init(child_node, node, spec->name, spec->type, child_opts);
}
}application (ie: lowlevel.c)
struct sol_flow_static_node_spec nodes[] = {
{SOL_FLOW_NODE_TYPE_GPIO_READER, "reader", &def_gpio_reader_opts.base},
{SOL_FLOW_NODE_TYPE_GPIO_WRITER, "writer", &def_gpio_writer_opts.base},
SOL_FLOW_STATIC_NODE_SPEC_GUARD
};
struct sol_flow_static_conn_spec conns[] = {
{ 0 /* reader */, 0, 1/* writer */, 0},
SOL_FLOW_STATIC_CONN_SPEC_GUARD
};
struct sol_flow_static_option_spec options[] = {
{0, /* other spec would come here */},
{1, /* other spec would come here */},
SOL_FLOW_STATIC_OPTION_SPEC_GUARD
};
struct sol_flow_static_spec spec = {
...
.nodes = nodes,
.conns = conns,
.exported_options = options,
...
};
type = sol_flow_static_new_type(&spec);
struct sol_flow_static_node_options myflow_opts = {
.base = {
.api_version = ..., .sub_api = SOL_FLOW_STATIC_NODE_OPTIONS_API_VERSION,
},
.children_options = {
{0, &gpio_reader_opts.base},
{1, &gpio_writer_opts.base},
{UINT16_MAX, NULL}
}
};
flow = sol_flow_node_new(parent, type, &myflow_opts.base);I'm assigning this to @ibriano as I remember he did the exported options, but maybe @cmarcelo may take a look.