On Fri, Feb 3, 2023 at 5:54 PM Daniel Burgener
<dburgener@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On 12/15/2022 4:34 PM, James Carter wrote:
> > Add the function cil_tree_remove_node() which takes a node pointer
> > as an input, finds the parent, walks the list of nodes to the node
> > prior to the given node, and then updates that nodes next pointer
> > to remove the given node from the tree.
> >
> > Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> > ---
> > libsepol/cil/src/cil_tree.c | 27 +++++++++++++++++++++++++++
> > libsepol/cil/src/cil_tree.h | 1 +
> > 2 files changed, 28 insertions(+)
> >
> > diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c
> > index 6376c208..73b4e135 100644
> > --- a/libsepol/cil/src/cil_tree.c
> > +++ b/libsepol/cil/src/cil_tree.c
> > @@ -248,6 +248,33 @@ void cil_tree_node_destroy(struct cil_tree_node **node)
> > *node = NULL;
> > }
> >
> > +void cil_tree_remove_node(struct cil_tree_node *node)
> > +{
> > + struct cil_tree_node *parent, *curr;
> > +
> > + if (node == NULL || node->parent == NULL) {
> > + return;
> > + }
> > +
> > + parent = node->parent;
> > +
> > + if (parent->cl_head == node) {
> > + parent->cl_head = node->next;
> > + return;
> > + }
> > +
> > + curr = parent->cl_head;
> > + while (curr && curr->next != node) {
> > + curr = curr->next;
> > + }
> > +
> > + if (curr == NULL) {
> > + return;
> > + }
> > +
> > + curr->next = node->next;
> > +}
> > +
>
> Is there a reason this leaves it to the caller to call
> cil_tree_node_destroy()? It just feels a little weird to leave the node
> unreachable without freeing. It looks like both callers in the series
> (cil_process_deny_rule(s)) call cil_tree_node_destroy() immediately after.
>
> -Daniel
>
Not that I can remember. I can't really think of a scenario where I
would want to remove a node, but not destroy it.
It also seems like it would be better to name it
cil_tree_node_remove() to fit the naming scheme of the other
functions.
Thanks,
Jim
> > /* Perform depth-first walk of the tree
> > Parameters:
> > start_node: root node to start walking from
> > diff --git a/libsepol/cil/src/cil_tree.h b/libsepol/cil/src/cil_tree.h
> > index 5a98da55..e4b3fd04 100644
> > --- a/libsepol/cil/src/cil_tree.h
> > +++ b/libsepol/cil/src/cil_tree.h
> > @@ -63,6 +63,7 @@ void cil_tree_children_destroy(struct cil_tree_node *node);
> >
> > void cil_tree_node_init(struct cil_tree_node **node);
> > void cil_tree_node_destroy(struct cil_tree_node **node);
> > +void cil_tree_remove_node(struct cil_tree_node *node);
> >
> > //finished values
> > #define CIL_TREE_SKIP_NOTHING 0
>
