On 4/5/20 10:55 AM, jer...@marvell.com wrote:
[...]
> diff --git a/lib/librte_graph/node.c b/lib/librte_graph/node.c
> index 336cd1c94..d04a0fce0 100644
> --- a/lib/librte_graph/node.c
> +++ b/lib/librte_graph/node.c
[...]
> +static rte_edge_t
> +edge_update(struct node *node, struct node *prev, rte_edge_t from,
> + const char **next_nodes, rte_edge_t nb_edges)
> +{
> + rte_edge_t i, max_edges, count = 0;
> + struct node *new_node;
> + bool need_realloc;
> + size_t sz;
> +
> + if (from == RTE_EDGE_ID_INVALID)
> + from = node->nb_edges;
> +
> + /* Don't create hole in next_nodes[] list */
> + if (from > node->nb_edges) {
> + rte_errno = ENOMEM;
> + goto fail;
> + }
> +
> + /* Remove me from list */
> + STAILQ_REMOVE(&node_list, node, node, next);
> +
> + /* Allocate the storage space for new node if required */
> + max_edges = from + nb_edges;
> + need_realloc = max_edges > node->nb_edges;
> + if (need_realloc) {
> + sz = sizeof(struct node) + (max_edges * RTE_NODE_NAMESIZE);
> + new_node = realloc(node, sz);
> + if (new_node == NULL) {
> + rte_errno = ENOMEM;
> + goto restore;
> + } else {
> + node = new_node;
> + }
> + }
> +
> + /* Update the new nodes name */
> + for (i = from; i < max_edges; i++, count++) {
> + if (rte_strscpy(node->next_nodes[i], next_nodes[count],
> + RTE_NODE_NAMESIZE) < 0) {
> + rte_errno = E2BIG;
> + goto restore;
> + }
> + }
> +restore:
> + /* Update the linked list to point new node address in prev node */
> + if (prev)
> + STAILQ_INSERT_AFTER(&node_list, prev, node, next);
> + else
> + STAILQ_INSERT_HEAD(&node_list, node, next);
AFAIU node_list keeps the list of nodes - so I guess you wanted here
"replace" the old node pointer with the new one. I have not yet seen
the usage of this function but it seems to me like you unconditionally
insert the updated node - possibly having node pointer present doubly or
with stale pointer. I might be missing something here.
> +
> + if (need_realloc)
> + node->nb_edges += max_edges;
It looks to me like this should be simple '='.
> +
> +fail:
> + return count;
> +}
[...]
> +rte_edge_t
> +rte_node_edge_update(rte_node_t id, rte_edge_t from, const char **next_nodes,
> + uint16_t nb_edges)
> +{
> + rte_edge_t rc = RTE_EDGE_ID_INVALID;
> + struct node *n, *prev;
> +
> + NODE_ID_CHECK(id);
> + graph_spinlock_lock();
> +
> + prev = NULL;
> + STAILQ_FOREACH(n, &node_list, next) {
> + if (n->id == id) {
> + rc = edge_update(n, prev, from, next_nodes, nb_edges);
> + break;
> + }
> + prev = n;
> + }
OK so in this context my comment above seems to be valid. When we find
the id we have: prev -> n, we call update() and in there we insert
new_node after prev so we end up with: prev -> n' -> n where n' might be
new address for n or just n when no realloc was performed.
Do I miss anything?
> +
> + graph_spinlock_unlock();
> +fail:
> + return rc;
> +}
> +
> +static rte_node_t
> +node_copy_edges(struct node *node, char *next_nodes[])
> +{
> + rte_edge_t i;
> +
> + for (i = 0; i < node->nb_edges; i++)
> + next_nodes[i] = node->next_nodes[i];
> +
> + return i;
> +}
> +
> +rte_node_t
> +rte_node_edge_get(rte_node_t id, char *next_nodes[])
> +{
> + rte_node_t rc = RTE_NODE_ID_INVALID;
> + struct node *node;
> +
> + NODE_ID_CHECK(id);
> + graph_spinlock_lock();
> +
> + STAILQ_FOREACH(node, &node_list, next) {
> + if (node->id == id) {
> + if (next_nodes == NULL)
> + rc = sizeof(char *) * node->nb_edges;
> + else
> + rc = node_copy_edges(node, next_nodes);
Do we want to ready for next_nodes not large enough?
> + break;
> + }
> + }
> +
> + graph_spinlock_unlock();
> +fail:
> + return rc;
> +}
[...]
With regards
Andrzej Ostruszka
