Re: [dpdk-dev] [PATCH v4 04/29] graph: implement node debug routines

[Andrzej Ostruszka <amo@semihalf.com>]

On 4/7/20 2:09 PM, Jerin Jacob wrote:
> On Tue, Apr 7, 2020 at 5:20 PM Andrzej Ostruszka <amo@semihalf.com> wrote:
>>
>> On 4/7/20 12:22 PM, Jerin Jacob wrote:
[...]
>>>>> +static void
>>>>> +node_scan_dump(FILE *f, rte_node_t id, bool all)
>>>>> +{
>>>>> +     struct node *node;
>>>>> +
>>>>> +     RTE_ASSERT(f != NULL);
>>>>
>>>> Why the assert?  Below this is used in public (I guess) functions so
>>>> user can provide wrong input - in that case I'd expect warning/error not
>>>> an assert.
>>>
>>> Public API rte_node_dump() and node_scan_dump() calls this API without
>>> any check.
>>
>> That was my point.  I would expect either there or here to have a check
>> for arg instead of assert.  I'd say that asserts are very good for
>> checking internal logic, but not so for checking if user input is OK.
> 
> All DPDK _dump() functions returns void. I thought, We will keep the same here.
> Another option is. if it is NULL we can return.
> i.e
> -RTE_ASSERT(f != NULL);
> +if (f == NULL)
> + return
> 
> Either scheme is OK with me, Let me know your preference, I will
> change accordingly.

No, I don't want to silently skip that.  Have an error message here and
return error but don't call rte_panic() which would abort application.
That would be my preference, but if you don't want to change return type
here (and where this is called) then I'm fine with what it is now.

With regards
Andrzej Ostruszka