Re: [PATCH v5 1/3] lib: introduce dispatcher library

[David Marchand <david.marchand@redhat.com>]

On Mon, Oct 9, 2023 at 6:50 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:

[snip]

> >>>> +static int
> >>>> +evd_set_service_runstate(struct rte_dispatcher *dispatcher, int state)
> >>>> +{
> >>>> +       int rc;
> >>>> +
> >>>> +       rc = rte_service_component_runstate_set(dispatcher->service_id,
> >>>> +                                               state);
> >>>> +
> >>>> +       if (rc != 0) {
> >>>> +               RTE_EDEV_LOG_ERR("Unexpected error %d occurred while setting "
> >>>> +                                "service component run state to %d\n", rc,
> >>>> +                                state);
> >>>> +               RTE_ASSERT(0);
> >>>
> >>> Why not propagating the error to callers?
> >>>
> >>>
> >>
> >> The root cause would be a programming error, hence an assertion is more
> >> appropriate way to deal with the situation.
> >
> > Without building RTE_ENABLE_ASSERT (disabled by default), the code
> > later in this function will still be executed.
> >
>
> If RTE_ASSERT() is not the way to assure a consistent internal library
> state, what is? RTE_VERIFY()?

The usual way in DPDK is to use RTE_VERIFY or rte_panic with the error message.
There is also libc assert().

RTE_ASSERT is more of a debug macro since it is under a build option.


But by making the library "panic" on some assertion, I have followup comments:
- what is the point of returning an int for rte_dispatcher_start() /
rte_dispatcher_stop()?
- rte_dispatcher_start() and rte_dispatcher_stop() (doxygen)
documentation needs updating, as they can't return anything but 0.


-- 
David Marchand