Repository navigation
Conversation
…tifunction=off read as on
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 10 minutes. View limit details
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note for reviewers: this is a proof of concept. It shows that the problem is real, reproducible and fixable with a small change; refining the code is welcome, and it was tested through the real VM form on Unraid 7.3.4 (see Tests). The more useful discussion, though, is why saving the VM form has been allowed to throw away settings people made by hand for years, although the change that introduced this code (#361, 2018-08-13) says its purpose was "keeping custom XML modifications done by the user while the VM gets updated via the GUI", and how Unraid would prefer to resolve it, with this approach or a better one. In particular:
// settings not in the GUI, but maybe customizedis still in the function, and the same report has been filed every year since 2017 (11 of the 12 years 2015-2026, figures below), while each later change made the form own one more section without saying that the leftovers should be discarded;Two small fixes in the VM manager, related to https://forums.unraid.net/topic/111226/ .
libvirt.php(hostdev read-back):$x ?? "" ? "on" : "off"parses as($x ?? "") ? "on" : "off", somultifunction='off'was shown as "on". It now returns "on" only formultifunction='on'.libvirt_helpers.phparray_update_recursive(): when a passthrough device in the form matches one in the stored XML, the stored entry is replaced by the regenerated one, so a guest<address>set by hand in XML view is lost on the next form save. The stored address is now kept when the form did not generate one. Values the form owns are unchanged: boot order and ROM come from the form as before, and a stored multifunction address is not kept (so switching Multifunction to Off still removes it). Devices removed in the form are still removed; new devices get nothing.Companion PR #2812 keeps the clock, sound, input and graphics settings the form has no field for; the two touch different lines and merge in either order.
Tests
multifunctionon/off/absent; a hand-set guest address is kept; an address the form sets wins; a device removed in the form is still removed; a new device gets nothing; switching Multifunction from On to Off drops the stored multifunction address. Master showsmultifunction='off'as "on" and drops the hand-set address.Background: prior reports and timeline
array_update_recursive()replaces a stored passthrough device by the regenerated one when the form contains the same device, and the form writes a guest<address>only for its own multifunction layout and a few Q35 cases. The multifunction read-back inlibvirt.phptests the attribute for truthiness. The list below is what was found in public threads, GitHub and the release notes.Pattern
multifunction='off'showing as On; that part is visible only in the code.->value ? "on" : "off", 83d02ea, PR VM multifunction support #1464, merged 2023-10-17), somultifunction='off'has been read as On since then; the current spellingnodeValue ?? "" ? "on" : "off"is from b844237 (2025-02-17). The hostdev replace inarray_update_recursive()dates from 2018-08-15 (345a640, part of VMs: preserve XML custom settings in VM config updates #361). Checked against webgui master 9754759 (2026-10-29).Timeline (items relevant to this PR)
nodeValue ?? "" ? "on" : "off"(same behaviour)Reports without a fix or staff response
dynamix.vm.managerfor preserve/persist/custom XML shows only unrelated changes).Official guidance
XMLgenerated by the WebGUI; switch toXMLmode for advanced edits." The passages I read do not say whether a later form save keeps those edits (the page was read in search-selected passages, about 8 of 25 thousand characters).array_update_recursive():// settings not in the GUI, but maybe customized.Impact
Why a fix in the current release line
Unraid 8 was announced on 2026-08-17 as a base-OS change ("Unraid 8 is moving to a Fedora base, built on Universal Blue's uCore."), with a closed beta, open beta and release candidates still to come and no release date ("it'll ship when it's ready", https://unraid.net/blog/unraid-8-announced). Unraid's support policy covers "the current public minor series" and "the previous public minor series" (https://docs.unraid.net/unraid-os/updating-unraid/release-types/); a change merged now reaches servers through those releases.