Skip to content

Conversation

@SimonFair
Copy link
Contributor

@SimonFair SimonFair commented Dec 29, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Improved stability of virtual machine template configuration handling. The system now properly validates the presence of network interfaces before attempting to modify their MAC address settings, preventing errors that could occur when NICs are not present in the template configuration. This enhancement ensures more reliable template operations across different scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 29, 2025

Walkthrough

A guard condition was added to the addtemplatexml function in the libvirt helpers to check for NIC existence before iterating and unsetting MAC addresses, replacing unconditional MAC removal that could reference absent indices.

Changes

Cohort / File(s) Summary
NIC MAC address removal guard
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php
Modified addtemplatexml to conditionally unset NIC MAC addresses: added existence check before iterating through NICs to prevent undefined index errors when NIC structure is absent.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 With guards we stand against the void,
Where absent NICs once caused us pain,
A check, a loop, bugs now destroyed—
MAC addresses won't cause us strain!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix: template remove MAC addresses for all interfaces.' accurately describes the main change: guarding NIC MAC address removal to properly iterate through all interfaces instead of blindly unsetting a potentially non-existent nic index.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 987fa42 and 677f0c1.

📒 Files selected for processing (1)
  • emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.

Applied to files:

  • emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php
🔇 Additional comments (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)

2676-2680: LGTM! Good defensive fix.

The guard condition properly prevents errors when templates lack NICs or when NICs don't have MAC addresses defined. This ensures that template creation proceeds safely in edge cases where the NIC structure may be incomplete.

The nested isset check on line 2678 is extra defensive but doesn't hurt—it ensures we only attempt to unset MAC addresses that actually exist.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@SimonFair SimonFair changed the title Fix: remove MAC addresses for all interfaces. Fix: template remove MAC addresses for all interfaces. Dec 29, 2025
@github-actions
Copy link

🔧 PR Test Plugin Available

A test plugin has been generated for this PR that includes the modified files.

Version: 2025.12.29.1902
Build: View Workflow Run

📥 Installation Instructions:

Install via Unraid Web UI:

  1. Go to Plugins → Install Plugin
  2. Copy and paste this URL:
https://preview.dl.unraid.net/pr-plugins/pr-2499/webgui-pr-2499.plg
  1. Click Install

Alternative: Direct Download

⚠️ Important Notes:

  • Testing only: This plugin is for testing PR changes
  • Backup included: Original files are automatically backed up
  • Easy removal: Files are restored when plugin is removed
  • Conflicts: Remove this plugin before installing production updates

📝 Modified Files:

Click to expand file list
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php

🔄 To Remove:

Navigate to Plugins → Installed Plugins and remove webgui-pr-2499, or run:

plugin remove webgui-pr-2499

🤖 This comment is automatically generated and will be updated with each new push to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant