watchcat: resolve logical interface names

[dhrm1k]

📦 Package Details

Maintainer: @danielfdickinson, @dhrm1k

Description:
Resolve logical interface handling in watchcat.

Fixes: #29829

watchcat historically expects the configured interface to be a real device name,
because it passes that value to ping -I. The restart path then maps that real
device back to the logical openwrt network before calling ifup.

This change also handles logical interface references like @wan / @3GMM.

For logical interface input:

@3GMM -> ping device from netifd
@3GMM -> restart network 3GMM

For existing real-device input, the old behavior is kept:

wwan0 -> ping device wwan0
wwan0 -> restart network resolved by find_config

It also treats the default ModemManager mm_iface_name value of null as empty,
so watchcat does not try to use "null" as a real modem interface name.

---

🧪 Run Testing Details

• OpenWrt Version: OpenWrt 25.12.2 r32802-f505120278
• OpenWrt Target/Subtarget: x86/64
• OpenWrt Device: VM

---

✅ Formalities

• I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

If your PR contains a patch:

• Not applicable, this PR does not add or modify an upstream source patch.

[danielfdickinson]

Aside from needing robust validation/sanitization of inputs that get evald and/or quote expanded, I think it looks good. I will run an automated review in the next few days.

[danielfdickinson]

Regarding eval: I'll lay odds the automated reviewers will (correctly) complain unless there is robust validation/sanitization of the variables and values involved.

[dhrm1k]

Good point. Same as I thought I agree the eval bit is the part I’m least
happy with here.

I used it mainly to keep the helper small, but I’m open to changing the shape if
there is a cleaner way to return both values. I’ll look at
removing it first; otherwise I’ll add strict validation before anything gets
expanded.

Would also be interested in your take on what you’d prefer here before I
push another revision.

[danielfdickinson]

@dhrm1k My preference would be avoiding eval, if possible.

I'm not sure I would say there is a cleaner way to return both values, but there are safer ways. One possibility is to not try to get both values from a single function, even though it increases the number calls required.

I think this is a case of no ideal solution, so pick what makes the most sense to you of the safer options.

[dhrm1k]

@danielfdickinson I also added both of us to PKG_MAINTAINER.

Happy to help keep an eye on watchcat issues and reviews from here and follow the package maintainer expectations.

[danielfdickinson]

@danielfdickinson I also added both of us to PKG_MAINTAINER.

Happy to help keep an eye on watchcat issues and reviews from here and follow the package maintainer expectations.

Thank you!

[dhrm1k]

I reworked it to avoid eval entirely.

The resolver is now split into two helpers: one returns the ping device and one
returns the restart network. It is a little more code, but it avoids indirect
assignment and keeps the call straightforward.

[danielfdickinson]

@dhrm1k Here is the result of the automated review I ran:

PR Review: watchcat: resolve logical interface names (096d69e) + watchcat: update package maintainers (045b375)

Summary

This PR introduces the ability for watchcat to accept @logical-interface references (e.g., @wan) in addition to bare device names (e.g., eth0) or network section names. It also updates package maintainers and bumps PKG_RELEASE.

---

1. Commits Reviewed

Commit	Description
045b37565	watchcat: update package maintainers
096d69e45	watchcat: resolve logical interface names

---

2. Architecture & Patterns

• Language: POSIX shell (/bin/sh)
• Key Files Changed:
  • utils/watchcat/Makefile — version bump + maintainer update
  • utils/watchcat/files/watchcat.sh — core logic changes
• External Dependencies Introduced: . /lib/functions/network.sh (adds network_get_device)
• Pattern: OpenWrt convention of @interface prefix for logical interface references
• Data Model: No schema/struct changes. New parameters passed through to watchcat_restart_network_iface().

---

3. Detailed Review Findings

✅ Positive Observations

1. Clean separation of concerns: watchcat_resolve_ping_iface() returns the device (for ping -I), while watchcat_resolve_restart_iface() returns the network section name (for ifup). This distinction is correct because ping -I requires a kernel device name while ifup requires a UCI network section name.

2. Backward compatibility: Non-@ prefixed inputs are handled identically to before (via find_config and network_get_device), so existing configs will continue to work.

3. TIMINGS.md alignment: The changes do not alter timing behavior. The reset_failure_timer logic documented in TIMINGS.md remains intact. The resolution calls are done at initialization time, not inside the monitoring loop, so no timing overhead is introduced per ping cycle.

4. mm_iface_name null normalization: The addition of [ "$mm_iface_name" = "null" ] && mm_iface_name="" inside watchcat_monitor_network() is a nice defensive fix, ensuring the restart_iface mode's default "null" value doesn't slip through to watchcat_restart_modemmanager_iface().

5. Maintainer update is clean: Standard metadata change, no issues.

---

⚠️ Issues & Concerns

Issue 1 (Moderate): watchcat_resolve_restart_iface() — @ path returns the network name, not the device

Location: Lines 54–64 of watchcat.sh (watchcat_resolve_restart_iface())

case "$iface" in
    @*)
        network="${iface#@}"
        [ -n "$network" ] || return 1
        if ! network_get_device device "$network"; then
            printf '%s\n' "$network"
            return 1
        fi
        printf '%s\n' "$network"    # <--- BUG: Returns "$network", not "$device"
        return 0
        ;;
esac

Problem: When a @logical interface is passed, the function calls network_get_device device "$network" to verify the device exists, but then discards the resolved $device value and instead returns $network (the stripped logical name like "wan"). This happens to work because the logical name like "wan" is typically also the UCI network section name accepted by ifup. However, this is fragile:

• If the logical interface name differs from the UCI network section name, ifup will fail.
• The network_get_device call is only used for validation (existence check), not for its return value. The intent of verifying the device exists is good, but the return value should arguably be consistent with the non-@ path.

Non-@ path for comparison (lines 67–76):

network="$(find_config "$iface")"
if [ -n "$network" ]; then
    printf '%s\n' "$network"     # Returns UCI section name — correct for ifup
    return 0
fi

if network_get_device device "$iface"; then
    printf '%s\n' "$iface"       # Returns the original input — potentially inconsistent
    return 0
fi

Recommendation: Document explicitly that the @ path returns the logical interface name (assumed to be the UCI section name) rather than the resolved device. Alternatively, consider whether find_config should also be attempted on the logical name after stripping @, which would be more robust:

case "$iface" in
    @*)
        network="${iface#@}"
        [ -n "$network" ] || return 1
        # Try to find the UCI network section for this logical name
        uci_network="$(find_config "$network")"
        if [ -n "$uci_network" ]; then
            printf '%s\n' "$uci_network"
            return 0
        fi
        # Fallback: if the logical name itself is a valid network section
        if network_get_device device "$network"; then
            printf '%s\n' "$network"
            return 0
        fi
        printf '%s\n' "$iface"
        return 1
        ;;
esac

---

Issue 2 (Moderate): watchcat_resolve_ping_iface() — @ path may return unresolved @logical on failure

Location: Lines 28–29 of watchcat.sh

printf '%s\n' "$iface"    # Prints "@wan" literally
return 1

Problem: When resolution fails for an @-prefixed input, the function returns the raw @wan string while returning exit code 1. The caller (watchcat_monitor_network) does:

ping_iface="$(watchcat_resolve_ping_iface "$iface")"

It captures stdout but ignores the exit code. So ping_iface will be set to "@wan" (the literal string), and later ping -I "@wan" will silently fail with an "invalid source address" error, but watchcat will continue running. The user will see silent ping failures.

Same issue exists in watchcat_resolve_restart_iface(): On failure, it prints the input and returns 1, but the caller also ignores the exit code.

Recommendation: Check the return code and fall back to the original behavior (i.e., use the raw input or skip the interface-specific path):

if [ "$iface" != "" ]; then
    ping_iface="$(watchcat_resolve_ping_iface "$iface")" || ping_iface="$iface"
    restart_iface="$(watchcat_resolve_restart_iface "$iface")" || restart_iface=""
fi

Or better yet, if resolution fails, log a warning so the operator is aware:

if [ "$iface" != "" ]; then
    if ! ping_iface="$(watchcat_resolve_ping_iface "$iface")"; then
        logger -p daemon.warn -t "watchcat[$$]" "Could not resolve interface '$iface' for pinging, falling back to raw input"
        ping_iface="$iface"
    fi
    if ! restart_iface="$(watchcat_resolve_restart_iface "$iface")"; then
        logger -p daemon.warn -t "watchcat[$$]" "Could not resolve interface '$iface' for restart, falling back to raw input"
        restart_iface="$iface"
    fi
fi

---

Issue 3 (Low): Unused network local variable in watchcat_resolve_ping_iface()

Location: Line 16 of watchcat.sh

local logical device network

The network variable is declared but never used in watchcat_resolve_ping_iface(). It is only used inside the function body in watchcat_resolve_restart_iface(). While not harmful (just a minor cleanliness issue), it suggests copy-paste derivation between the two functions.

Recommendation: Remove network from the local declaration:

local logical device

---

Issue 4 (Low): Resolution happens once at startup, not re-evaluated

Location: Lines 195–198 and 290–291 of watchcat.sh

if [ "$iface" != "" ]; then
    ping_iface="$(watchcat_resolve_ping_iface "$iface")"
    restart_iface="$(watchcat_resolve_restart_iface "$iface")"
fi

Observation: The interface resolution is computed once at monitor startup and cached in ping_iface / restart_iface. If the network configuration changes at runtime (e.g., the device backing @wan changes from eth0 to wwan0), watchcat will not pick up the change until restarted.

Assessment: This is likely acceptable behavior — watchcat is restarted via /etc/init.d/watchcat start after recovery actions, and UCI network changes typically trigger a service restart anyway. However, it may be worth documenting this implicit assumption.

---

Issue 5 (Nit): watchcat_resolve_ping_iface() — non-@ path falls through to network_get_device but find_config might have already matched

Location: Lines 33–42 of watchcat.sh

network="$(find_config "$iface")"
if [ -n "$network" ]; then
    printf '%s\n' "$iface"    # Returns original input
    return 0
fi

if network_get_device device "$iface"; then
    printf '%s\n' "$device"   # Returns resolved device
    return 0
fi

When find_config succeeds (meaning the input is a valid network section name), the function returns the original input. But ping -I expects a kernel device name, not a UCI section name. So ping -I "wan" would fail with an error because "wan" is not a device.

This is the same behavior as before this PR (since find_config was used implicitly in the old code path via watchcat_restart_network_iface), but now it is exposed more directly because ping -I is called with the result of watchcat_resolve_ping_iface().

However: Looking more carefully at the pre-PR code, the old behavior was:

if [ "$iface" != "" ]; then
    ping_result="$(ping ... -I "$iface" ...)"

It used the raw $iface directly, not through any resolution. So this behavior is unchanged — if a user passed "wan" as the interface, ping -I "wan" would fail before this PR too. This is pre-existing behavior, not a regression, but the new code makes the same assumption explicit.

Recommendation: No action required, but it may be worth noting that users should pass device names (e.g., eth0) or @logical references, not bare UCI network section names, for the ping source address.

---

Issue 6 (Nit): watchcat_ping() function only resolves for ping, not for restart

Location: Lines 291–292 of watchcat.sh

ping_iface=""
[ "$iface" != "" ] && ping_iface="$(watchcat_resolve_ping_iface "$iface")"

The watchcat_ping() function (used by ping_reboot mode) resolves the ping interface but not the restart interface. This is correct because ping_reboot mode calls reboot_now() directly rather than watchcat_restart_network_iface(). No issue, just noting for completeness.

---

4. Summary of Findings

#	Severity	Description	Status
1	Moderate	watchcat_resolve_restart_iface() @ path returns logical name instead of trying find_config first	Needs review
2	Moderate	Exit codes from resolve functions are ignored; failures result in silent @name strings passed to ping -I	Should fix
3	Low	Unused network local variable in watchcat_resolve_ping_iface()	Cleanup
4	Low	Resolution is startup-only; no runtime re-resolution	Acceptable, may document
5	Nit	Non-@ path passes UCI section names to ping -I; pre-existing behavior	No action
6	Nit	watchcat_ping() resolves ping only; correct for reboot-only mode	No action

---

5. Verdict

The PR is fundamentally sound and addresses a real need — supporting @logical interface names in watchcat. The core design of separating ping-target resolution (device name) from restart-target resolution (network name) is correct.

However, Issue 2 (ignoring exit codes from resolve functions) should be addressed before merge, as it can lead to silent failures where @logical references that don't resolve are passed literally to ping -I, producing confusing error logs. Issue 1 should be reviewed to determine if the @ path's return value is intentionally the logical name or if find_config should be attempted first for robustness.

Recommendation: Request changes for Issues 1 and 2. Issues 3-5 are cosmetic/minor.

[danielfdickinson]

Sometimes the reviewer changes it mind on whether something is a bug, midstream, so reading parts like Issue 5 can be confusing.

[dhrm1k]

Yeah, i see. issue 5 confused me for a minute too, but it looks like it ends up being old behavior, not something this PR newly broke!?

I made a small update for the actual problem from the review. If @... cannot be resolved now, watchcat logs it and does not pass the raw @... value to ping -I. (Issue 2)

I also checked the network local from issue 3. It is still used in the ping resolver, so I left that alone.

[danielfdickinson]

Yes, issue 5 is pre-existing and not necessarily a bug so much as something to document: This is pre-existing behavior, not a regression, but the new code makes the same assumption explicit.

Recommendation: No action required, but it may be worth noting that users should pass device names (e.g., eth0) or @logical references, not bare UCI network section names, for the ping source address.

Sounds good on the fixes. Will review (manually) tomorrow.

[danielfdickinson]

The code looks good. I've poked the original issue reporter to see if they are able and willing to verify the fix.

[dhrm1k]

The original reporter does not seem very active

[danielfdickinson]

@dhrm1k That's normal. I was hoping they would/will respond so that there would independent run testing, which would hopefully help get this merged faster.

[openwrt-ai]

Reviewed 2 new commits; no new issues found.

---

Generated by Claude Code