Copilot commented on code in PR #12981:
URL: https://github.com/apache/cloudstack/pull/12981#discussion_r3050683197
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -882,10 +989,7 @@ for LINE in "${LINES[@]}"; do
if [[ ${#vlist[@]} -eq 0 && ${#flist[@]} -eq 0 ]]; then
FP_ENABLED=1
fi
Review Comment:
`parse_pci_address` assigns `DOMAIN/BUS/SLOT/FUNC` via bash dynamic scoping;
these call sites don’t declare those variables `local` first, so the values can
leak into the global scope (and between loop iterations), making future
changes/debugging brittle. Declare `local DOMAIN BUS SLOT FUNC` in the
immediate scope before calling `parse_pci_address` (as is already done in
`process_mdev_instances`), or refactor `parse_pci_address` to return values
explicitly (e.g., via stdout or nameref parameters).
```suggestion
fi
local DOMAIN BUS SLOT FUNC
```
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -791,36 +892,35 @@ for LINE in "${LINES[@]}"; do
PF_MAX_RESOLUTION_Y=$MAX_RESOLUTION_Y
# === full_passthrough usage ===
- raw="${pci_to_vm[$PCI_ADDR]:-}"
+ # pci_to_vm is keyed by the full domain-qualified form, so normalize the
+ # lspci-style PCI_ADDR (which may be short for domain 0) before lookup.
+ raw="${pci_to_vm[$(normalize_pci_address "$PCI_ADDR")]:-}"
FULL_USED_JSON=$(to_json_vm "$raw")
# === vGPU (MDEV) instances ===
VGPU_ARRAY="[]"
declare -a vlist=()
# Process mdev on the Physical Function
- MDEV_BASE="/sys/bus/pci/devices/0000:$PCI_ADDR/mdev_supported_types"
+ MDEV_BASE="/sys/bus/pci/devices/$SYSFS_ADDR/mdev_supported_types"
process_mdev_instances "$MDEV_BASE" "$PCI_ADDR"
# === VF instances (SR-IOV / MIG) ===
VF_ARRAY="[]"
declare -a flist=()
if ((TOTALVFS > 0)); then
- for VF_LINK in /sys/bus/pci/devices/0000:"$PCI_ADDR"/virtfn*; do
+ for VF_LINK in /sys/bus/pci/devices/"$SYSFS_ADDR"/virtfn*; do
[[ -L $VF_LINK ]] || continue
VF_PATH=$(readlink -f "$VF_LINK")
- VF_ADDR=${VF_PATH##*/} # e.g. "0000:65:00.2"
- VF_BDF="${VF_ADDR:5}" # "65:00.2"
+ # Keep the full domain-qualified address (e.g.
"0000:65:00.2")
+ VF_BDF=${VF_PATH##*/}
# For NVIDIA SR-IOV, check for vGPU (mdev) on the VF
itself
if [[ "$VENDOR_ID" == "10de" ]]; then
VF_MDEV_BASE="$VF_PATH/mdev_supported_types"
process_mdev_instances "$VF_MDEV_BASE" "$VF_BDF"
fi
Review Comment:
`parse_pci_address` assigns `DOMAIN/BUS/SLOT/FUNC` via bash dynamic scoping;
these call sites don’t declare those variables `local` first, so the values can
leak into the global scope (and between loop iterations), making future
changes/debugging brittle. Declare `local DOMAIN BUS SLOT FUNC` in the
immediate scope before calling `parse_pci_address` (as is already done in
`process_mdev_instances`), or refactor `parse_pci_address` to return values
explicitly (e.g., via stdout or nameref parameters).
```suggestion
local DOMAIN BUS SLOT FUNC
```
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -613,16 +701,21 @@ for VM in "${VMS[@]}"; do
continue
fi
- # -- PCI hostdevs: use xmlstarlet to extract BDF for all PCI host
devices --
- while read -r bus slot func; do
+ # -- PCI hostdevs: use xmlstarlet to extract the full
domain:bus:slot.func
+ # for all PCI host devices. libvirt's <address> element may omit the
domain
+ # attribute, in which case we default to 0.
+ while read -r dom bus slot func; do
[[ -n "$bus" && -n "$slot" && -n "$func" ]] || continue
- # Format to match lspci output (e.g., 01:00.0) by padding with
zeros
+ [[ -n "$dom" ]] || dom="0"
+ # Format to match lspci -D output (e.g., 0000:01:00.0) by
padding with zeros
+ dom_fmt=$(printf "%04x" "0x$dom")
bus_fmt=$(printf "%02x" "0x$bus")
slot_fmt=$(printf "%02x" "0x$slot")
func_fmt=$(printf "%x" "0x$func")
- BDF="$bus_fmt:$slot_fmt.$func_fmt"
+ BDF="${dom_fmt}:${bus_fmt}:${slot_fmt}.${func_fmt}"
pci_to_vm["$BDF"]="$VM"
done < <(echo "$xml" | xmlstarlet sel -T -t -m
"//hostdev[@type='pci']/source/address" \
+ -v "substring-after(@domain, '0x')" -o " " \
-v "substring-after(@bus, '0x')" -o " " \
-v "substring-after(@slot, '0x')" -o " " \
-v "substring-after(@function, '0x')" -n 2>/dev/null || true)
Review Comment:
`xmlstarlet` emits an empty first field when `@domain` is missing; because
`read` collapses leading whitespace, `dom` will incorrectly receive the bus
value and `func` becomes empty, causing `[[ -n "$bus" && -n "$slot" && -n
"$func" ]]` to fail and the hostdev mapping to be skipped. Fix by making the
selector always output a domain token (e.g., defaulting to `0000` in the XPath
output), or by adjusting the parsing logic to detect the 3-field case and treat
it as “domain missing”.
--
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]
