On 2023-10-11 16:57, David Marchand wrote:
On Mon, Oct 9, 2023 at 6:50 PM Mattias Rönnblom <hof...@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.
Those return vales are purely there for reasons of symmetry, or maybe
you can call it API consistent, with Eventdev APIs (e.g., the event
ethernet RX adapter). I guess that's less of an issue now, when it's a
separate library, but still relevant, since the programmer will be
familiar with those APIs.
You could also argue that in the future, there may be errors which needs
to be signaled to the caller. But that's a weak argument, since we don't
know exactly what those would be (and thus they can't be documented), so
it's still an API/ABI change, even though the function signature doesn't
change.
Now I'm leaning toward removing the return value. Please advise. Over.
