Re: [PATCH 1/9] libsepol/cil: Use struct cil_db * instead of void *

[Petr Lautrbach <lautrbach@redhat.com>]

Daniel Burgener <dburgener@linux.microsoft.com> writes:

> On 9/27/2023 4:41 PM, James Carter wrote:
>> On Wed, Sep 27, 2023 at 3:27 PM Daniel Burgener
>> <dburgener@linux.microsoft.com> wrote:
>>>
>>>> @@ -3661,21 +3615,17 @@ static int cil_check_for_bad_inheritance(struct cil_tree_node *node)
>>>>        return rc;
>>>>    }
>>>>
>>>> -static int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
>>>> +static int __cil_resolve_ast_node(struct cil_tree_node *node, struct cil_args_resolve *args)
>>>>    {
>>>>        int rc = SEPOL_OK;
>>>> -     struct cil_args_resolve *args = extra_args;
>>>> +     struct cil_db *db = args->db;
>>>>        enum cil_pass pass = 0;
>>>>
>>>> -     if (node == NULL || args == NULL) {
>>>> -             goto exit;
>>>> -     }
>>>> -
>>>
>>> Is deleting the "node == NULL" part of this check intended here?  It
>>> seems unrelated to the rest of the commit, and it's not locally obvious
>>> that it's safe.
>> 
>> You are right. It is not related to the rest of the commit. There are
>> a bunch of these sorts of checks that are useless and really annoy me.
>> The function __cil_resolve_ast_node() is called once from
>> __cil_resolve_ast_node_helper() and neither node nor args can be NULL.
>> Since I was changing something nearby, I guess I couldn't resist. I
>> can leave it in, if people prefer. It doesn't cause any harm, other
>> than annoying me.
>> 
>
> As is is fine by me.  Your explanation makes sense.  I mostly wanted to 
> make sure it was reasoned out rather than an accidental drop, but now 
> that you point it out, it does look impossible for this to be NULL.
>
> Reviewed-by: Daniel Burgener <dburgener@linux.microsoft.com>

Acked-by: Petr Lautrbach <lautrbach@redhat.com>


Petr