Re: [PATCH v4 00/11] devtools, lib, test, net, common: remove unused rte_bitmap_free() and plt_bitmap_free()

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun, 22 Dec 2024 13:49:20 +0100
Ariel Otilibili <otilibil@eurecom.fr> 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 <otilibil@eurecom.fr>  
**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: stable@dpdk.org | ✅ 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: stable@dpdk.org | ✅ 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: stable@dpdk.org | ⚠️ **WARNING** | Has Fixes tag but `Cc: stable@dpdk.org` 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: stable@dpdk.org` 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.