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

[Thomas Monjalon <thomas@monjalon.net>]

06/10/2023 10:46, David Marchand:
> On Thu, Oct 5, 2023 at 12:09 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> > >> +static int
> > >> +evd_lookup_handler_idx(struct rte_dispatcher_lcore *lcore,
> > >> +                      const struct rte_event *event)
> > >
> > > Wrt DPDK coding tyle, indent is a single tab.
> > > Adding an extra tab is recommended when continuing control statements
> > > like if()/for()/..
> > >
> >
> > Sure, but I don't understand why you mention this.
> 
> I wanted to remind the DPDK coding style which I try to more closely
> enforce for new code.
> indent is off in this file (especially for function prototypes with
> multiple tabs used).
> 
> >
> > > On the other hand, max accepted length for a line is 100 columns.
> > >
> > > Wdyt of a single line for this specific case?
> >
> >
> > Are you asking why the evd_lookup_handler_idx() function prototype is
> > not a single line?
> >
> > It would make it long, that's why. Even if 100 wide lines are allowed,
> > it doesn't means the author is forced to use such long lines?
> 
> I find it more readable.
> If you want to stick to 80 columns, please comply with a single tab for indent.

I think this is a case of continuation line, so it should be 2 tabs.
We can make it clear in the doc.


> > >> +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.

Don't forget you are writing a library, so you shouldn't stop runtime
because of an error. It is always better to return errors.
Assert should be used only for debugging, that's why it is disabled by default.