[PWCI] "Assign all unit tests to suites" #535
Conversation
For the fast tests, we have two extra parameters specifying when the test can be run without hugepages or using ASan. The true/false nature of these parameters is not very clear, so change things so that they are explicitly specified as NOHUGE_OK/NOHUGE_SKIP and ASAN_OK/ASAN_SKIP instead. Explicitly validate the options in the meson.build files, rather than just checking for one of the pair of options - which can hide errors. Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Acked-by: Marat Khalili <marat.khalili@huawei.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
The shift of a negative number (or very large positive) is undefined behaviour which causes errors when run with UBSan. Fix this by making the behaviour explicit for the edge case of n being zero in the calculation. Fixes: de3cfa2 ("sched: initial import") Cc: stable@dpdk.org Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Acked-by: Marat Khalili <marat.khalili@huawei.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reduce the timer duration and the primary wait duration in the secondary process timer autotest. This should help ensure it doesn't time out what run in a CI environment. Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
When the secondary process run from timer_secontary_autotest fails, the timer loop is never stopped so the whole process hangs until timeout. Fix this by setting the stop flag before checking for success or failure of the secondary process. Fixes: 50247fe ("test/timer: exercise new APIs in secondary process") Cc: stable@dpdk.org Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Add APIs for internal DPDK use to allow querying the paths of drivers loaded into the running instance. Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
For unit tests which run secondary processes, allow passing the driver paths used by the primary process to that secondary. This allows use of mempools in those secondary tests. Without this, any tests using mempools in secondary process will fail in shared builds. Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
If the power subsystem cannot be initialized e.g. due to not being root, skip the capabilities test, rather than failing it. Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
There are quite a number of test cases defined which are not present in any test-suite. Meson warns about these on build, so reduce the warnings by adding external-mem, ipsec-sad, power-caps and secondary timer test cases to the fast-test suite. Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
A number of eventdev, cryptodev and ethdev tests require devices to be present in order to run so add them to the "driver" suite of tests. Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
If there are no tests which are not assigned to a test suite, omit the line printing the non_suite_tests. This avoid having error messages from meson about test "" not being in any test suite. Fixes: 25065ef ("test: emit warning for orphaned tests") Cc: stable@dpdk.org Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Acked-by: Marat Khalili <marat.khalili@huawei.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Define an "attic" test suite for unstable or unmaintained tests, and move the red autotest to that suite. Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideIntroduce driver-path iteration APIs and propagate -d/--driver-path arguments to secondary test processes, while refactoring fast test registration to use explicit NOHUGE_/ASAN_ flags and assigning tests into suites with minor timing and correctness fixes in timers, RED, and power tests. Sequence diagram for driver loading and driver path iterationsequenceDiagram
actor App
participant EAL
participant ArgsParser
participant PluginDirScanner
participant SharedDriverList
App->>EAL: rte_eal_init(argc, argv)
EAL->>ArgsParser: eal_parse_args()
ArgsParser-->>ArgsParser: collect -d and --driver-path arguments
loop for each driver_path_arg
ArgsParser->>EAL: eal_plugin_add(path, true)
EAL->>SharedDriverList: allocate shared_driver
EAL-->>SharedDriverList: set name, lib_handle, from_cmdline = true
end
EAL->>PluginDirScanner: eal_plugindir_init(default_solib_dir)
PluginDirScanner-->>PluginDirScanner: scan directory for .so files
loop for each sopath
PluginDirScanner->>EAL: eal_plugin_add(sopath, false)
EAL->>SharedDriverList: allocate shared_driver
EAL-->>SharedDriverList: set name, lib_handle, from_cmdline = false
end
App->>EAL: rte_eal_driver_path_count(cmdline_only)
EAL->>SharedDriverList: iterate and count based on from_cmdline and cmdline_only
EAL-->>App: count
App->>EAL: rte_eal_driver_path_next(NULL, cmdline_only)
EAL->>SharedDriverList: get first matching shared_driver
EAL-->>App: path
loop while path != NULL
App->>EAL: rte_eal_driver_path_next(path, cmdline_only)
EAL->>SharedDriverList: find current and next matching entry
EAL-->>App: next path or NULL
end
Class diagram for EAL shared_driver and driver path iteration APIsclassDiagram
class shared_driver {
+char name[PATH_MAX]
+void* lib_handle
+bool from_cmdline
}
class solib_list {
+shared_driver* head
+shared_driver* tail
}
class eal_common_options {
+int eal_plugin_add(const char* path, bool from_cmdline)
+void eal_plugindir_init(const char* path)
+void eal_plugins_init()
+int eal_parse_args()
}
class rte_eal_driver_path_api {
+const char* rte_eal_driver_path_next(const char* start, bool cmdline_only)
+unsigned int rte_eal_driver_path_count(bool cmdline_only)
+macro RTE_EAL_DRIVER_PATH_FOREACH(path, cmdline_only)
}
solib_list "1" o-- "*" shared_driver : contains
eal_common_options --> solib_list : manages
eal_common_options ..> shared_driver : allocates_and_initializes
rte_eal_driver_path_api --> solib_list : iterates
rte_eal_driver_path_api ..> shared_driver : reads_name_and_from_cmdline
Flow diagram for rte_eal_driver_path_next iteration logicflowchart TD
A["Call rte_eal_driver_path_next(start, cmdline_only)"] --> B{start is NULL?}
B -->|Yes| C["solib = first element of solib_list"]
B -->|No| D["Find element in solib_list where start == solib.name"]
D --> E["solib = next element after matched one"]
C --> F{solib is NULL?}
E --> F
F -->|Yes| G["Return NULL"]
F -->|No| H{cmdline_only is true?}
H -->|No| I["Return solib.name"]
H -->|Yes| J{solib.from_cmdline is true?}
J -->|Yes| I
J -->|No| K["solib = next element in solib_list"] --> L{solib is NULL?}
L -->|Yes| G
L -->|No| J
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughInfrastructure changes introduce driver path support in test process utilities, add test parameter validation in the build system, and implement environment-specific test registration flags (NOHUGE_OK/SKIP, ASAN_OK/SKIP) replacing boolean parameters across the test suite, along with new test registration macros. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (107)
⛔ Files not processed due to max files limit (14)
🧰 Additional context used🧬 Code graph analysis (1)app/test/process.h (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (114)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/eal/common/eal_common_options.c:758-766` </location>
<code_context>
+RTE_EXPORT_SYMBOL(rte_eal_driver_path_next)
+const char *
+rte_eal_driver_path_next(const char *start, bool cmdline_only)
+{
+ struct shared_driver *solib;
+
+ if (start == NULL) {
+ solib = TAILQ_FIRST(&solib_list);
+ } else {
+ /* Find the current entry based on the name string */
+ TAILQ_FOREACH(solib, &solib_list, next) {
+ if (start == solib->name) {
+ solib = TAILQ_NEXT(solib, next);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Pointer comparison for `start` requires callers to pass the exact returned pointer, which is a subtle contract.
This lookup assumes `start` is the exact same pointer as one of the stored `solib->name` values (i.e., a value previously returned by this function), not just an equal string. That’s safe with `RTE_EAL_DRIVER_PATH_FOREACH`, but easy to misuse for other callers. If this is meant as a public/ABI-stable API, consider either documenting that `start` must be a value previously returned by this function, or switching to an opaque iterator type (handle/index) instead of `const char *` to make correct use clearer.
Suggested implementation:
```c
/*
* Iterate over registered shared drivers.
*
* The iterator is driven by the return value of this function:
* - To start iteration, callers must pass start == NULL.
* - To advance, callers must pass as start a pointer value that was
* previously returned by this function.
*
* Note: the lookup below compares the 'start' argument to the stored
* solib->name using pointer equality, not strcmp(). This is intentional
* to keep the API ABI-stable without introducing an explicit iterator
* handle type, but it means arbitrary strings must not be passed here.
*
* The RTE_EAL_DRIVER_PATH_FOREACH macro is implemented to respect this
* contract and should be preferred by most callers.
*/
RTE_EXPORT_SYMBOL(rte_eal_driver_path_next)
const char *
rte_eal_driver_path_next(const char *start, bool cmdline_only)
```
```c
} else {
/*
* Find the current entry based on the previously returned
* name pointer. This relies on pointer identity, not on
* string comparison, so 'start' must be a value returned
* by this function (or by RTE_EAL_DRIVER_PATH_FOREACH).
*/
TAILQ_FOREACH(solib, &solib_list, next) {
```
</issue_to_address>
### Comment 2
<location> `app/test/process.h:42-43` </location>
<code_context>
#define PREFIX_ALLOW "--allow="
+#define PREFIX_DRIVER_PATH "--driver-path="
static int
add_parameter_allow(char **argv, int max_capacity)
</code_context>
<issue_to_address>
**suggestion (testing):** Add explicit tests for driver-path propagation to child processes
Given the new reliance on `rte_eal_driver_path_count(true)` and `RTE_EAL_DRIVER_PATH_FOREACH(..., true)` for propagating `--driver-path`, please add an end-to-end test (e.g., in `test_eal_flags.c` or a new EAL test) that:
- Launches a process with multiple `-d` / `--driver-path` options (plus optional directory-based drivers),
- Confirms a `process_dup()` child receives equivalent `--driver-path=` arguments reconstructed from `rte_eal_driver_path_*`,
- Exercises zero, single, and multiple paths, and verifies that only `from_cmdline` entries are propagated.
This will validate both the iterator APIs and the helper used to rebuild the arguments.
Suggested implementation:
```c
RTE_EAL_DRIVER_PATH_FOREACH(driver_path, true) {
size_t len;
char *arg;
if (count >= max_capacity)
break;
len = strlen(PREFIX_DRIVER_PATH) + strlen(driver_path) + 1;
arg = malloc(len);
if (arg == NULL)
break;
snprintf(arg, len, "%s%s", PREFIX_DRIVER_PATH, driver_path);
argv[count++] = arg;
}
```
To fully implement your test suggestion, you will also need to:
1. Ensure `app/test/process.h` (or a common header) includes the required headers if they are not already present:
- `#include <stdlib.h>` for `malloc`
- `#include <string.h>` for `strlen`
- `#include <stdio.h>` for `snprintf`
2. Add an end-to-end EAL test (e.g., in `app/test/test_eal_flags.c` or an appropriate EAL test file) that:
- Starts a primary process with:
- Zero `-d/--driver-path` options,
- A single `--driver-path=...` option,
- Multiple `--driver-path=...` options plus any directory-based drivers configured via other APIs.
- From within that process, calls `process_dup()` (or the existing helper you use for child processes).
- Asserts that the child’s `argv`:
- Contains `--driver-path=<path>` entries that exactly match the `from_cmdline` paths returned by `RTE_EAL_DRIVER_PATH_FOREACH(driver_path, true)`,
- Does not contain directory-based / non-`from_cmdline` driver paths,
- Preserves the order of the original `--driver-path` arguments.
- Verifies behavior when `rte_eal_driver_path_count(true)` returns:
- `0` (no `--driver-path` added),
- `1`,
- `>1`.
3. In those tests, explicitly validate that:
- The iterator APIs (`rte_eal_driver_path_count(true)` and `RTE_EAL_DRIVER_PATH_FOREACH(..., true)`) return the expected set of paths.
- The rebuilt `--driver-path=` arguments in the child’s `argv` are obtained exclusively via the above helper, not by manually pushing flags.
These additional changes will exercise both the iterator APIs and the helper that reconstructs `--driver-path` arguments, satisfying the review comment.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| rte_eal_driver_path_next(const char *start, bool cmdline_only) | ||
| { | ||
| struct shared_driver *solib; | ||
|
|
||
| if (start == NULL) { | ||
| solib = TAILQ_FIRST(&solib_list); | ||
| } else { | ||
| /* Find the current entry based on the name string */ | ||
| TAILQ_FOREACH(solib, &solib_list, next) { |
There was a problem hiding this comment.
suggestion (bug_risk): Pointer comparison for start requires callers to pass the exact returned pointer, which is a subtle contract.
This lookup assumes start is the exact same pointer as one of the stored solib->name values (i.e., a value previously returned by this function), not just an equal string. That’s safe with RTE_EAL_DRIVER_PATH_FOREACH, but easy to misuse for other callers. If this is meant as a public/ABI-stable API, consider either documenting that start must be a value previously returned by this function, or switching to an opaque iterator type (handle/index) instead of const char * to make correct use clearer.
Suggested implementation:
/*
* Iterate over registered shared drivers.
*
* The iterator is driven by the return value of this function:
* - To start iteration, callers must pass start == NULL.
* - To advance, callers must pass as start a pointer value that was
* previously returned by this function.
*
* Note: the lookup below compares the 'start' argument to the stored
* solib->name using pointer equality, not strcmp(). This is intentional
* to keep the API ABI-stable without introducing an explicit iterator
* handle type, but it means arbitrary strings must not be passed here.
*
* The RTE_EAL_DRIVER_PATH_FOREACH macro is implemented to respect this
* contract and should be preferred by most callers.
*/
RTE_EXPORT_SYMBOL(rte_eal_driver_path_next)
const char *
rte_eal_driver_path_next(const char *start, bool cmdline_only) } else {
/*
* Find the current entry based on the previously returned
* name pointer. This relies on pointer identity, not on
* string comparison, so 'start' must be a value returned
* by this function (or by RTE_EAL_DRIVER_PATH_FOREACH).
*/
TAILQ_FOREACH(solib, &solib_list, next) {
Auto-submission for "http://patchwork.dpdk.org/project/dpdk/list/?series=36861"
Summary by Sourcery
Introduce richer EAL driver path introspection and propagate driver-path flags to child test processes, while tightening fast-test metadata and classification to ensure all unit tests are assigned to appropriate suites.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Summary by CodeRabbit
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.