On 2023-10-06 11:03, Thomas Monjalon wrote:
06/10/2023 10:46, David Marchand:
On Thu, Oct 5, 2023 at 12:09 PM Mattias Rönnblom <hof...@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.
A breach of the API contract should always be met with an assertion -
library or not. That is the most helpful thing you can do, so that one
fails early and programmer can go fix the bug.
The use of EINVAL and attempts to return error in the face of API
violations is a bad practice, in my opinion. What could the program
possible do, if it learns it's using a function the wrong way? If the
two memory areas in memmove() overlap, should there be a error code, so
that caller can retry but "please not with overlapping memory regions
this time".
I know that the EINVAL pattern is wide-spread practice in DPDK, at the
cost of performance and complexity, I would argue. I guess this practice
originates from the kernel/libc tradition of validating system calls (a
place where this can be actually be done in a reliable manner, unlike in
normal user code).
