Copilot commented on code in PR #12981:
URL: https://github.com/apache/cloudstack/pull/12981#discussion_r3050364677
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -506,10 +545,52 @@ get_nvidia_profile_info() {
fi
}
-# Get nodedev name for a PCI address (e.g. "00:02.0" -> "pci_0000_00_02_0")
-get_nodedev_name() {
+# Parse a PCI address and assign the libvirt-formatted DOMAIN/BUS/SLOT/FUNC
+# values into the caller's scope. Accepts both full ("0000:00:02.0") and short
+# ("00:02.0") formats; short addresses are assumed to be in PCI domain 0.
+#
+# IMPORTANT: bash uses dynamic scoping, so callers that don't want these four
+# values to leak into the global scope MUST declare them `local` before
+# invoking this function, e.g.:
+# local DOMAIN BUS SLOT FUNC
+# parse_pci_address "$addr"
+parse_pci_address() {
+ local addr="$1"
+ if [[ $addr =~
^([0-9A-Fa-f]+):([0-9A-Fa-f]+):([0-9A-Fa-f]+)\.([0-9A-Fa-f]+)$ ]]; then
+ DOMAIN="0x${BASH_REMATCH[1]}"
+ BUS="0x${BASH_REMATCH[2]}"
+ SLOT="0x${BASH_REMATCH[3]}"
+ FUNC="0x${BASH_REMATCH[4]}"
+ elif [[ $addr =~ ^([0-9A-Fa-f]+):([0-9A-Fa-f]+)\.([0-9A-Fa-f]+)$ ]];
then
+ DOMAIN="0x0000"
+ BUS="0x${BASH_REMATCH[1]}"
+ SLOT="0x${BASH_REMATCH[2]}"
+ FUNC="0x${BASH_REMATCH[3]}"
+ else
+ DOMAIN="0x0000"
+ BUS="0x00"
+ SLOT="0x00"
+ FUNC="0x0"
+ fi
+}
Review Comment:
`parse_pci_address` claims to produce libvirt-formatted DOMAIN/BUS/SLOT/FUNC
values, but it only prefixes `0x` and preserves the original widths/casing.
This can emit non-canonical values (e.g. `0x1`, `0xAF`) which makes downstream
comparisons/debugging harder and can reintroduce inconsistencies between JSON
output and other normalization logic. Consider formatting each segment to fixed
widths (domain 4, bus/slot 2, function 1) and lowercasing, or adjust the
function/docs to match the actual behavior.
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -506,10 +545,52 @@ get_nvidia_profile_info() {
fi
}
-# Get nodedev name for a PCI address (e.g. "00:02.0" -> "pci_0000_00_02_0")
-get_nodedev_name() {
+# Parse a PCI address and assign the libvirt-formatted DOMAIN/BUS/SLOT/FUNC
+# values into the caller's scope. Accepts both full ("0000:00:02.0") and short
+# ("00:02.0") formats; short addresses are assumed to be in PCI domain 0.
+#
+# IMPORTANT: bash uses dynamic scoping, so callers that don't want these four
+# values to leak into the global scope MUST declare them `local` before
+# invoking this function, e.g.:
+# local DOMAIN BUS SLOT FUNC
+# parse_pci_address "$addr"
+parse_pci_address() {
+ local addr="$1"
+ if [[ $addr =~
^([0-9A-Fa-f]+):([0-9A-Fa-f]+):([0-9A-Fa-f]+)\.([0-9A-Fa-f]+)$ ]]; then
+ DOMAIN="0x${BASH_REMATCH[1]}"
+ BUS="0x${BASH_REMATCH[2]}"
+ SLOT="0x${BASH_REMATCH[3]}"
+ FUNC="0x${BASH_REMATCH[4]}"
+ elif [[ $addr =~ ^([0-9A-Fa-f]+):([0-9A-Fa-f]+)\.([0-9A-Fa-f]+)$ ]];
then
+ DOMAIN="0x0000"
+ BUS="0x${BASH_REMATCH[1]}"
+ SLOT="0x${BASH_REMATCH[2]}"
+ FUNC="0x${BASH_REMATCH[3]}"
+ else
+ DOMAIN="0x0000"
+ BUS="0x00"
+ SLOT="0x00"
+ FUNC="0x0"
+ fi
+}
+
+# Normalize a PCI address to its full domain-qualified form ("dddd:bb:ss.f").
+# Short addresses are widened by prepending domain "0000".
+normalize_pci_address() {
local addr="$1"
- echo "pci_$(echo "$addr" | sed 's/[:.]/\_/g' | sed 's/^/0000_/')"
+ if [[ $addr =~ ^[0-9A-Fa-f]+:[0-9A-Fa-f]+:[0-9A-Fa-f]+\.[0-9A-Fa-f]+$
]]; then
+ echo "$addr"
+ else
+ echo "0000:$addr"
+ fi
+}
Review Comment:
The `normalize_pci_address` docstring says it normalizes to the canonical
full form (`dddd:bb:ss.f`), but the implementation only prepends `0000:` for
short addresses and otherwise echoes the input unchanged. If an upstream source
ever provides an unpadded domain (e.g. `1:65:00.0`) or mixed-case segments,
this can break map lookups (`pci_to_vm`) and sysfs path generation. Suggest
making this function truly canonical (pad domain/bus/slot and lowercase) using
the same parsing/printf approach used elsewhere, or update the comment to
reflect the weaker guarantee.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtGpuDef.java:
##########
@@ -79,26 +79,45 @@ private void generatePciXml(StringBuilder gpuBuilder) {
gpuBuilder.append(" <driver name='vfio'/>\n");
gpuBuilder.append(" <source>\n");
- // Parse the bus address (e.g., 00:02.0) into domain, bus, slot,
function
- String domain = "0x0000";
- String bus = "0x00";
- String slot = "0x00";
- String function = "0x0";
+ // Parse the bus address into domain, bus, slot, function. Two input
formats are accepted:
+ // - "dddd:bb:ss.f" full PCI address with domain (e.g. 0000:00:02.0)
+ // - "bb:ss.f" legacy short BDF; domain defaults to 0000
+ // Each segment is parsed as a hex integer and formatted with fixed
widths
+ // (domain: 4 hex digits, bus/slot: 2 hex digits, function: 1 hex
digit) to
+ // produce canonical libvirt XML values regardless of input casing or
padding.
+ int domainVal = 0, busVal = 0, slotVal = 0, funcVal = 0;
if (busAddress != null && !busAddress.isEmpty()) {
String[] parts = busAddress.split(":");
- if (parts.length > 1) {
- bus = "0x" + parts[0];
- String[] slotFunctionParts = parts[1].split("\\.");
- if (slotFunctionParts.length > 0) {
- slot = "0x" + slotFunctionParts[0];
- if (slotFunctionParts.length > 1) {
- function = "0x" + slotFunctionParts[1].trim();
+ String slotFunction = null;
+ try {
+ if (parts.length == 3) {
+ domainVal = Integer.parseInt(parts[0], 16);
+ busVal = Integer.parseInt(parts[1], 16);
+ slotFunction = parts[2];
+ } else if (parts.length == 2) {
+ busVal = Integer.parseInt(parts[0], 16);
+ slotFunction = parts[1];
+ }
+ if (slotFunction != null) {
+ String[] slotFunctionParts = slotFunction.split("\\.");
+ if (slotFunctionParts.length > 0) {
+ slotVal = Integer.parseInt(slotFunctionParts[0], 16);
+ if (slotFunctionParts.length > 1) {
+ funcVal =
Integer.parseInt(slotFunctionParts[1].trim(), 16);
+ }
}
}
+ } catch (NumberFormatException e) {
+ // leave all values at 0 (safe defaults)
}
}
+ String domain = String.format("0x%04x", domainVal);
+ String bus = String.format("0x%02x", busVal);
+ String slot = String.format("0x%02x", slotVal);
+ String function = String.format("0x%x", funcVal);
+
gpuBuilder.append(" <address domain='").append(domain).append("'
bus='").append(bus).append("' slot='")
.append(slot).append("'
function='").append(function.trim()).append("'/>\n");
gpuBuilder.append(" </source>\n");
Review Comment:
`generatePciXml` silently falls back to `0000:00:00.0` when parsing fails
(NumberFormatException or unexpected `busAddress` shape). That can mask bad
input and potentially attach the wrong device or fail later with a libvirt
error that’s harder to trace. Prefer failing fast with a clear
exception/message that includes the original `busAddress` (or at least logging
a warning and skipping the device) so invalid GPU addresses don’t result in a
misleading default address.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
