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

[Jerin Jacob]

On Tue, Apr 7, 2020 at 5:20 PM Andrzej Ostruszka <amo at semihalf.com> wrote:
>
> On 4/7/20 12:22 PM, Jerin Jacob wrote:
> > On Mon, Apr 6, 2020 at 11:47 PM Andrzej Ostruszka <amo at semihalf.com> wrote:
> >>
> >> On 4/5/20 10:55 AM, jerinj at marvell.com wrote:
> >>> From: Jerin Jacob <jerinj at marvell.com>
> >>>
> >>> Adding node debug API implementation support to dump
> >>> single or all the node objects to the given file.
> >>>
> >>> Signed-off-by: Jerin Jacob <jerinj at marvell.com>
> >>> Signed-off-by: Kiran Kumar K <kirankumark at marvell.com>
> >>> Signed-off-by: Pavan Nikhilesh <pbhagavatula at marvell.com>
> >>> Signed-off-by: Nithin Dabilpuram <ndabilpuram at marvell.com>
> >> [...]
> >>> diff --git a/lib/librte_graph/node.c b/lib/librte_graph/node.c
> >>> index d04a0fce0..8592c1221 100644
> >>> --- a/lib/librte_graph/node.c
> >>> +++ b/lib/librte_graph/node.c
> >>> @@ -377,6 +377,38 @@ rte_node_edge_get(rte_node_t id, char *next_nodes[])
> >>>       return rc;
> >>>  }
> >>>
> >>> +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.

>
> But I'm fine if you ignore this.
>
> With regards
> Andrzej Ostruszka