Re: [RFC PATCH 3/9] libsepol/cil: Add cil_tree_remove_node function

[James Carter <jwcart2@gmail.com>]

On Fri, Feb 3, 2023 at 5:54 PM Daniel Burgener
<dburgener@linux.microsoft.com> 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@gmail.com>
> > ---
> >   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
>