Re: [PATCH] bridge: tighten VLAN parsing

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 31 Jul 2026 22:40:51 -0400
William Gonzalez <gonzalez.williamalexander1@gmail.com> wrote:

> ---

It is good to see more input validation but this patch fails the basic
requirements of iproute2 submissions.

1. No Signed-off-by
2. No commit message
3. No explanation

Longer AI generated review.

Subject: Re: [PATCH] bridge: tighten VLAN parsing

On Fri, 31 Jul 2026 22:40:51 -0400, William Gonzalez wrote:

This patch cannot be accepted in its current form due to missing required elements.

## BLOCKING ISSUES

### 1. Missing Commit Message (REQUIRED)

The patch has no commit description at all. This is unacceptable for iproute2.
Every patch MUST have:
- A clear subject line
- A description of what problem is being solved
- An explanation of the approach taken
- Examples of before/after behavior if applicable
- Signed-off-by line (DCO compliance)

### 2. Missing DCO Sign-off

The patch lacks a Signed-off-by line, which is mandatory for DCO (Developer
Certificate of Origin) compliance. All patches must include:

```
Signed-off-by: William Gonzalez <gonzalez.williamalexander1@gmail.com>
```

This certifies that you have the right to submit the patch under the GPL-2.0+
license and that you agree to the Developer Certificate of Origin.

## Technical Review

The technical changes (replacing atoi() with validated parsing) are good, but
there are issues that need addressing:

### VLAN 0 Handling Issue

The get_vlan() function rejects VLAN ID 0:

```c
if (*val < 1 || *val > 4094)
	return -1;
```

VLAN ID 0 is valid for 802.1Q priority-tagged frames. The patch needs to
either:
1. Document why VLAN 0 is rejected (if intentional)
2. Support VLAN 0 where appropriate
3. Have different validators for different contexts

### Memory Allocation

Using malloc() for parsing 4-digit numbers is unnecessary:

```c
start_arg = malloc(len + 1);
```

Better alternatives:
1. Use `strdupa()` to avoid malloc/free handling:
```c
char *start_arg = strdupa(arg);
start_arg[len] = '\0';
```

2. Or use a fixed stack buffer:
```c
char start_arg[8];  /* max "4094" plus null */
```

The `strdupa()` approach is cleaner as it avoids both malloc failure handling
and fixed buffer size checks.

### Range Validation Change

The patch changes behavior for ranges like "100-100" (single VLAN as range).
This needs to be documented or fixed.

## Required for Resubmission

1. **Add proper commit message** explaining:
   - What problem this solves
   - Why atoi() is inadequate
   - What the new validation provides
   - Any behavior changes

2. **Add Signed-off-by line** for DCO compliance

3. **Address VLAN 0 handling** - either support it or document why not

4. **Remove unnecessary malloc()** - use stack buffer

5. **Clarify range validation** changes

Example of acceptable commit message:

```
bridge: tighten VLAN parsing

The current VLAN ID parsing uses atoi() which silently accepts
invalid input like "123abc" and treats it as 123. This can cause
user confusion and unexpected behavior.

Replace atoi() with get_vlan() helper that:
- Validates the entire string is numeric
- Ensures VLAN ID is in valid range (1-4094)
- Provides clear error messages for invalid input

Note: VLAN 0 is not accepted as [explain rationale].

Before: bridge fdb add ... vlan 123x  (silently uses 123)
After:  bridge fdb add ... vlan 123x  (error: invalid vlan)

Signed-off-by: William Gonzalez <gonzalez.williamalexander1@gmail.com>
```

Please resubmit with these required elements. The core idea is good but the
patch needs proper documentation and a few technical adjustments.

NACK - missing commit message and DCO sign-off