On Sun, 22 Dec 2024 13:49:20 +0100
Ariel Otilibili <[email protected]> wrote:
> Hello,
>
> The first version was about the clearing of Coverity IDs 357712 & 357737;
> now this series is about the removal of rte_bitmap_free() and its alias
> plt_bitmap_free().
>
> As of commit 07604f2644 ("maintainers: update for next-net tree"):
>
> * rte_bitmap_free() returns an integer, and does nothing else
> * all functions that call rte_bitmap_free() do not use this return value.
>
> Details are in d5941e7269 ("devtools/cocci,lib/eal: remove unused
> rte_bitmap_free()").
>
> Looking forward your feedback,
Not sure why this got ignored. Probably because it hit at the wrong time in the
development cycle.
Since it ends up being an ABI change, it would need to happen on the next .11
release (ie 26.11)
and would need to have a deprecation step. Ideally do a patch to remove all
uses of the function
in an earlier release like 26.03 or 26.07 and add a release note, then drop it
in 26.11
Would also need to be rebased, since code base has changed.
Automated patch review also raised some concerns.
## Patch Series Review: Remove unused rte_bitmap_free()
**Series Author:** Ariel Otilibili <[email protected]>
**Version:** v4
**Patches:** 11
### Series Overview
This series removes the no-op `rte_bitmap_free()` function and all its callers
across the DPDK codebase. The function only performed a NULL check and
returned, making it functionally useless and causing Coverity warnings about
unused return values.
---
### Patch 1/11: `devtools/cocci, lib/eal: remove unused rte_bitmap_free()`
**Subject Line Analysis:**
| Check | Status | Notes |
|-------|--------|-------|
| Length ≤60 chars | ❌ **ERROR** | Subject is 53 chars but contains a comma |
| Punctuation | ❌ **ERROR** | Contains comma (`,`) which is forbidden per
AGENTS.md |
| Format | ⚠️ **WARNING** | Dual-component subjects should typically be
separate patches |
**Commit Body:**
| Check | Status |
|-------|--------|
| Line wrap ≤75 chars | ✅ OK |
| Doesn't start with "It" | ✅ OK |
| Signed-off-by present | ✅ OK |
| Coverity issue tag | ✅ OK (properly formatted: `Coverity issue: 357712,
357737`) |
**Tag Order:**
| Check | Status | Notes |
|-------|--------|-------|
| Coverity before Fixes | ✅ OK | N/A - no Fixes tag in this patch |
| Cc: [email protected] | ✅ OK | Present (in commit notes after `---`) |
**Code Review:**
- ✅ Removes the no-op function correctly
- ✅ Removes the cocci rule that references this function
- ⚠️ **WARNING**: This is an ABI-breaking change (removes a public API). Should
have release notes in `doc/guides/rel_notes/`.
---
### Patch 2/11: `app/test: remove unused rte_bitmap_free()`
**Subject Line Analysis:**
| Check | Status |
|-------|--------|
| Length ≤60 chars | ✅ OK (42 chars) |
| Lowercase after colon | ✅ OK |
| No trailing period | ✅ OK |
| No forbidden punctuation | ✅ OK |
| Correct prefix | ✅ OK (`app/test:`) |
**Commit Body:**
| Check | Status | Notes |
|-------|--------|-------|
| Line wrap ≤75 chars | ✅ OK | |
| Doesn't start with "It" | ✅ OK | |
| Signed-off-by | ✅ OK | |
| Coverity issue tag | ✅ OK | |
| Fixes tag format | ✅ OK | 12-char SHA with exact subject |
| Cc: [email protected] | ✅ OK | |
**Dependency Note:**
- Uses informal `Depends on` format in commit body. DPDK guidelines suggest
`Depends-on: series-NNNNN ("Title")` in commit notes after `---`.
---
### Patch 3/11: `net/sfc: remove unused rte_bitmap_free()`
**Subject Line:** ✅ OK
**Commit Body Issues:**
| Check | Status | Notes |
|-------|--------|-------|
| Cc: [email protected] | ⚠️ **WARNING** | Has Fixes tag but `Cc:
[email protected]` is only in commit notes, not the commit message body proper |
| Signed-off-by | ✅ OK | |
| Fixes tag | ✅ OK | |
---
### Patch 4/11: `crypto/ionic: remove unused rte_bitmap_free()`
**Subject Line:** ✅ OK
**Commit Body:** ✅ OK - All required elements present.
---
### Patch 5/11: `net/cxgbe: remove unused rte_bitmap_free()`
**Subject Line:** ✅ OK
**Commit Body:** ✅ OK
---
### Patch 6/11: `net/mlx4: remove unused rte_bitmap_free()`
**Subject Line:** ✅ OK
**Commit Body:** ✅ OK
---
### Patch 7/11: `common/mlx5: remove unused rte_bitmap_free()`
**Subject Line:** ✅ OK
**Commit Body:** ✅ OK
---
### Patch 8/11: `net/bonding: remove unused rte_bitmap_free()`
**Subject Line:** ✅ OK
**Commit Body:** ✅ OK
---
### Patch 9/11: `net/netvsc: remove unused rte_bitmap_free()`
**Subject Line:** ✅ OK
**Commit Body:** ✅ OK
---
### Patch 10/11: `common/cnxk: remove unused plt_bitmap_free()`
**Subject Line:** ✅ OK
**Commit Body:** ✅ OK - Good explanation of why `plt_bitmap_free` is being
removed (it's an alias).
---
### Patch 11/11: `{common,net}/cnxk: remove unused plt_bitmap_free()`
**Subject Line Analysis:**
| Check | Status | Notes |
|-------|--------|-------|
| Length ≤60 chars | ✅ OK (54 chars) |
| Punctuation | ❌ **ERROR** | Contains comma (`,`) inside braces, which is
forbidden |
**Commit Body:**
| Check | Status | Notes |
|-------|--------|-------|
| Depends-on format | ⚠️ **WARNING** | Uses informal `Depends on commits:`
format instead of proper `Depends-on: series-NNNNN` |
| Multiple Fixes tags | ✅ OK | Multiple Fixes tags are acceptable |
| Signed-off-by | ✅ OK | |
**Code Review:**
- ⚠️ **WARNING** (Patch 11, `roc_npa.c`): Removing the `goto bmap_free` label
and associated code creates a situation where if `plt_zmalloc` fails, the
function falls through without returning an error. The error handling path
looks broken:
```c
if (lf->npa_qint_mem == NULL) {
rc = NPA_ERR_ALLOC;
- goto bmap_free;
}
```
After this change, `rc` is set but execution continues to the next allocation.
This appears to be a **bug**.
---
## Summary
### Errors (Must Fix)
1. **Patch 1/11**: Subject line contains forbidden comma punctuation
2. **Patch 11/11**: Subject line contains forbidden comma punctuation
3. **Patch 11/11**: Potential bug in `roc_npa.c` - error handling path appears
broken after removing the `goto bmap_free` label
### Warnings (Should Fix)
1. **Patch 1/11**: Missing release notes for ABI-breaking change (removal of
public API `rte_bitmap_free`)
2. **Patches 2-11**: Informal dependency notation; consider using `Depends-on:
series-NNNNN ("Title")` format in commit notes
3. **All patches with Fixes tags**: `Cc: [email protected]` appears in commit
notes after `---` rather than in the commit message body (though this is a
common practice and may be acceptable)
### Info (Consider)
- The series is well-structured with clear progression: first remove the
function definition, then remove all callers
- Good use of Coverity issue tags where applicable
- Thorough identification of all callers across the codebase
### Recommended Actions
1. Fix subject lines in patches 1 and 11 to avoid commas:
- Patch 1: Split into two patches, or use `devtools/cocci: remove
rte_bitmap_free cocci rule` and a separate patch for EAL
- Patch 11: Use `common/cnxk: remove unused plt_bitmap_free()` and include
net/cnxk changes with it (or split into two patches)
2. Verify the error handling in `roc_npa.c` (patch 11) - the removal of `goto
bmap_free` appears to break error handling when `plt_zmalloc` fails.
3. Add release notes for the removal of `rte_bitmap_free()` API.
