Update unsupported qemu models in XMLs#2625
Draft
SimonFair wants to merge 4 commits intounraid:masterfrom
Draft
Conversation
Contributor
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant Init as "rc.libvirt (init script)"
participant XML as "/etc/libvirt/qemu/*.xml"
participant Libvirtd as "libvirtd_start"
participant Vendor as "Vendor ID Conversion"
Init->>XML: scan domain XML files
Init->>XML: update_legacy_machine_types() — rewrite pc-*-<major>.<minor> to pc-*-8.0 if major < 8
Init->>Libvirtd: continue libvirtd_start
Libvirtd->>XML: apply vendor_id conversion and other XML edits
Libvirtd->>Libvirtd: start libvirt services
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@etc/rc.d/rc.libvirt`:
- Around line 184-190: The sed replacement is too broad and only handles single
quotes and an unescaped $MACHINE; update the logic so the sed invocation matches
machine attributes with either single or double quotes and only replaces the
numeric version suffix (e.g. -N.N) rather than the whole $MACHINE string, and
ensure any regex-special characters from $MACHINE/PREFIX are treated literally
(escape them or use a safer capture-based pattern). Concretely, modify the sed
call referenced by the sed -ri line to use a regex like matching
machine=(["'])PREFIX-[0-9]+\.[0-9]+\1 and replace only the -X.Y portion with
-8.0 (preserving the original quote capture), leaving $XML and variables
MACHINE, PREFIX, MAJOR, and the grep-derived input logic intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@etc/rc.d/rc.libvirt`:
- Around line 187-190: The sed expression under the MAJOR < 8 branch is too
broad and replaces any "${PREFIX}-X.Y" regardless of X; update the sed regex in
the block that defines ESC_PREFIX and calls sed -ri so it only matches major
versions 0–7 (e.g. use ${ESC_PREFIX}-[0-7]\.[0-9]+) and preserves the
surrounding quote via the existing capture groups; in short, change the pattern
machine=([\"'])${ESC_PREFIX}-[0-9]+\.[0-9]+\1 to
machine=\1${ESC_PREFIX}-[0-7]\.[0-9]+\1 (or equivalent) so only legacy <8
entries are rewritten to -8.0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
etc/rc.d/rc.libvirt (1)
177-191: Avoid repeated full-file rewrites for the same machine prefix.When a file has multiple legacy versions of one prefix (e.g.,
pc-q35-5.2andpc-q35-7.1), this loop can run the samesedscan multiple times. Track processed prefixes and rewrite once per prefix.Proposed refactor
update_legacy_machine_types(){ local XML=$1 local MACHINE= local PREFIX= local MAJOR= + local -A UPDATED_PREFIXES=() while IFS= read -r MACHINE; do [[ $MACHINE =~ ^((pc-(i440fx|q35)))-([0-9]+)\.([0-9]+)$ ]] || continue PREFIX=${BASH_REMATCH[1]} MAJOR=${BASH_REMATCH[4]} - if (( MAJOR < 8 )); then + if (( MAJOR < 8 )) && [[ -z ${UPDATED_PREFIXES[$PREFIX]} ]]; then local ESC_PREFIX=${PREFIX//./\\.} sed -ri "s/machine=([\"'])${ESC_PREFIX}-[0-7]\.[0-9]+\1/machine=\1${ESC_PREFIX}-8.0\1/g" "$XML" + UPDATED_PREFIXES[$PREFIX]=1 fi done < <(grep -oP "machine=[\"']pc-(?:i440fx|q35)-[0-9]+\.[0-9]+[\"']" "$XML" | grep -oP "pc-(?:i440fx|q35)-[0-9]+\.[0-9]+" | sort -u) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etc/rc.d/rc.libvirt` around lines 177 - 191, The loop in update_legacy_machine_types repeatedly runs the same sed replacement for the same machine PREFIX (e.g., pc-q35) when multiple legacy versions exist; modify update_legacy_machine_types to track processed prefixes (e.g., using a bash associative array or a simple string/set) and skip handling a PREFIX if already processed, so only a single sed invocation per unique PREFIX/ESC_PREFIX is done on the given XML; keep variables XML, PREFIX, MAJOR, ESC_PREFIX and the existing grep pipeline but add the prefix-tracking check before computing ESC_PREFIX and calling sed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@etc/rc.d/rc.libvirt`:
- Around line 177-191: The loop in update_legacy_machine_types repeatedly runs
the same sed replacement for the same machine PREFIX (e.g., pc-q35) when
multiple legacy versions exist; modify update_legacy_machine_types to track
processed prefixes (e.g., using a bash associative array or a simple string/set)
and skip handling a PREFIX if already processed, so only a single sed invocation
per unique PREFIX/ESC_PREFIX is done on the given XML; keep variables XML,
PREFIX, MAJOR, ESC_PREFIX and the existing grep pipeline but add the
prefix-tracking check before computing ESC_PREFIX and calling sed.
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
1 participant
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.
This change is to update the minimum support q35 of i440fx model in the XML.
Support of older values are being dropped. This will update all values < 8.0 to 8.0
These are still supported but deprecated so will be removed in future updates. The current best level is 8.0
root@computenode:~# virsh capabilities | grep "deprecated='yes'>pc-"
pc-q35-5.2
pc-i440fx-6.2
pc-i440fx-5.2
pc-q35-7.1
pc-q35-6.1
pc-i440fx-7.1
pc-q35-5.1
pc-i440fx-6.1
pc-i440fx-5.1
pc-q35-7.0
pc-q35-6.0
pc-i440fx-7.0
pc-q35-5.0
pc-q35-7.2
pc-i440fx-6.0
pc-i440fx-5.0
pc-q35-6.2
pc-i440fx-7.2
pc-q35-5.2
pc-i440fx-6.2
pc-i440fx-5.2
pc-q35-7.1
pc-q35-6.1
pc-i440fx-7.1
pc-q35-5.1
pc-i440fx-6.1
pc-i440fx-5.1
pc-q35-7.0
pc-q35-6.0
pc-i440fx-7.0
pc-q35-5.0
pc-q35-7.2
pc-i440fx-6.0
pc-i440fx-5.0
pc-q35-6.2
pc-i440fx-7.2
Summary by CodeRabbit