Copilot commented on code in PR #12981:
URL: https://github.com/apache/cloudstack/pull/12981#discussion_r3051105555
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -506,10 +541,58 @@ 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"
- 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
+ DOMAIN=$(printf "0x%04x" "0x${BASH_REMATCH[1]}")
+ BUS=$(printf "0x%02x" "0x${BASH_REMATCH[2]}")
+ SLOT=$(printf "0x%02x" "0x${BASH_REMATCH[3]}")
+ FUNC=$(printf "0x%x" "0x${BASH_REMATCH[4]}")
+ elif [[ $addr =~ ^([0-9A-Fa-f]+):([0-9A-Fa-f]+)\.([0-9A-Fa-f]+)$ ]];
then
+ DOMAIN="0x0000"
+ BUS=$(printf "0x%02x" "0x${BASH_REMATCH[1]}")
+ SLOT=$(printf "0x%02x" "0x${BASH_REMATCH[2]}")
+ FUNC=$(printf "0x%x" "0x${BASH_REMATCH[3]}")
+ else
+ DOMAIN="0x0000"
+ BUS="0x00"
+ SLOT="0x00"
+ FUNC="0x0"
+ fi
+}
+
+# Normalize a PCI address to its canonical full domain-qualified form
+# ("dddd:bb:ss.f", lowercase, zero-padded). Both "dddd:bb:ss.f" (full) and
+# "bb:ss.f" (short, domain 0) inputs are accepted.
+normalize_pci_address() {
+ local addr="$1"
+ if [[ $addr =~
^([0-9A-Fa-f]+):([0-9A-Fa-f]+):([0-9A-Fa-f]+)\.([0-9A-Fa-f]+)$ ]]; then
+ printf "%04x:%02x:%02x.%x\n" \
+ "0x${BASH_REMATCH[1]}" "0x${BASH_REMATCH[2]}" \
+ "0x${BASH_REMATCH[3]}" "0x${BASH_REMATCH[4]}"
+ elif [[ $addr =~ ^([0-9A-Fa-f]+):([0-9A-Fa-f]+)\.([0-9A-Fa-f]+)$ ]];
then
+ printf "0000:%02x:%02x.%x\n" \
+ "0x${BASH_REMATCH[1]}" "0x${BASH_REMATCH[2]}"
"0x${BASH_REMATCH[3]}"
+ else
+ echo "$addr"
+ fi
+}
Review Comment:
The comment promises canonical, lowercase, zero-padded output, but the
fallback path returns the input unchanged (including casing/padding). Either
update the comment to reflect that the function only normalizes recognized
formats, or change the fallback to enforce the documented behavior (e.g.,
lowercase), or return a failure when the format is unrecognized.
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -781,7 +882,7 @@ for LINE in "${LINES[@]}"; do
read -r TOTALVFS NUMVFS < <(get_sriov_counts "$PCI_ADDR")
Review Comment:
`SYSFS_ADDR` is introduced specifically because sysfs paths require a full
domain-qualified address, but `get_sriov_counts` is still called with
`PCI_ADDR` (which can be short for domain 0, and may be non-zero
domain-qualified). If `get_sriov_counts` uses sysfs paths internally (as the
surrounding code does), this will break SR-IOV detection on non-zero domains.
Prefer passing `SYSFS_ADDR` (or a normalized form) into `get_sriov_counts`, or
have `get_sriov_counts` normalize internally.
```suggestion
read -r TOTALVFS NUMVFS < <(get_sriov_counts "$SYSFS_ADDR")
```
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -613,18 +701,23 @@ 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 IFS=: read -r dom bus slot func; do
Review Comment:
`dom_fmt`/`bus_fmt`/`slot_fmt`/`func_fmt` are assigned inside the loop but
not declared `local`. In large bash scripts this increases the risk of
accidental reuse/clobbering (especially given the script already relies on
dynamic scoping elsewhere). Declaring these as `local` within the loop body (or
pre-declaring them as local before the loop) would reduce side effects.
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -613,18 +701,23 @@ 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 IFS=: 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")
Review Comment:
`dom_fmt`/`bus_fmt`/`slot_fmt`/`func_fmt` are assigned inside the loop but
not declared `local`. In large bash scripts this increases the risk of
accidental reuse/clobbering (especially given the script already relies on
dynamic scoping elsewhere). Declaring these as `local` within the loop body (or
pre-declaring them as local before the loop) would reduce side effects.
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -506,10 +541,58 @@ 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"
- 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
+ DOMAIN=$(printf "0x%04x" "0x${BASH_REMATCH[1]}")
+ BUS=$(printf "0x%02x" "0x${BASH_REMATCH[2]}")
+ SLOT=$(printf "0x%02x" "0x${BASH_REMATCH[3]}")
+ FUNC=$(printf "0x%x" "0x${BASH_REMATCH[4]}")
+ elif [[ $addr =~ ^([0-9A-Fa-f]+):([0-9A-Fa-f]+)\.([0-9A-Fa-f]+)$ ]];
then
Review Comment:
The regexes accept variable-length hex segments for
domain/bus/slot/function. This can yield non-canonical output (e.g., bus/slot
wider than 2 hex digits, function wider than 1) because `%02x` is a minimum
width, not a maximum, and it can mask invalid values rather than rejecting
them. Tighten the accepted formats (e.g., domain `{4}` or `{8}`, bus/slot
`{2}`, function `{1}`) and/or validate ranges (domain ≤ `ffff`, bus/slot ≤
`ff`, function ≤ `7`) to ensure generated libvirt and cache keys stay valid.
```suggestion
if [[ $addr =~
^([0-9A-Fa-f]{4}):([0-9A-Fa-f]{2}):([0-9A-Fa-f]{2})\.([0-7])$ ]]; then
DOMAIN=$(printf "0x%04x" "0x${BASH_REMATCH[1]}")
BUS=$(printf "0x%02x" "0x${BASH_REMATCH[2]}")
SLOT=$(printf "0x%02x" "0x${BASH_REMATCH[3]}")
FUNC=$(printf "0x%x" "0x${BASH_REMATCH[4]}")
elif [[ $addr =~ ^([0-9A-Fa-f]{2}):([0-9A-Fa-f]{2})\.([0-7])$ ]]; then
```
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -506,10 +541,58 @@ 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"
- 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
+ DOMAIN=$(printf "0x%04x" "0x${BASH_REMATCH[1]}")
+ BUS=$(printf "0x%02x" "0x${BASH_REMATCH[2]}")
+ SLOT=$(printf "0x%02x" "0x${BASH_REMATCH[3]}")
+ FUNC=$(printf "0x%x" "0x${BASH_REMATCH[4]}")
+ elif [[ $addr =~ ^([0-9A-Fa-f]+):([0-9A-Fa-f]+)\.([0-9A-Fa-f]+)$ ]];
then
+ DOMAIN="0x0000"
+ BUS=$(printf "0x%02x" "0x${BASH_REMATCH[1]}")
+ SLOT=$(printf "0x%02x" "0x${BASH_REMATCH[2]}")
+ FUNC=$(printf "0x%x" "0x${BASH_REMATCH[3]}")
+ else
+ DOMAIN="0x0000"
+ BUS="0x00"
+ SLOT="0x00"
+ FUNC="0x0"
+ fi
Review Comment:
The regexes accept variable-length hex segments for
domain/bus/slot/function. This can yield non-canonical output (e.g., bus/slot
wider than 2 hex digits, function wider than 1) because `%02x` is a minimum
width, not a maximum, and it can mask invalid values rather than rejecting
them. Tighten the accepted formats (e.g., domain `{4}` or `{8}`, bus/slot
`{2}`, function `{1}`) and/or validate ranges (domain ≤ `ffff`, bus/slot ≤
`ff`, function ≤ `7`) to ensure generated libvirt and cache keys stay valid.
```suggestion
local domain_num bus_num slot_num func_num
if [[ $addr =~
^([0-9A-Fa-f]{4}):([0-9A-Fa-f]{2}):([0-9A-Fa-f]{2})\.([0-9A-Fa-f]{1})$ ]]; then
domain_num=$((16#${BASH_REMATCH[1]}))
bus_num=$((16#${BASH_REMATCH[2]}))
slot_num=$((16#${BASH_REMATCH[3]}))
func_num=$((16#${BASH_REMATCH[4]}))
if (( domain_num <= 0xffff && bus_num <= 0xff && slot_num <=
0xff && func_num <= 7 )); then
DOMAIN=$(printf "0x%04x" "$domain_num")
BUS=$(printf "0x%02x" "$bus_num")
SLOT=$(printf "0x%02x" "$slot_num")
FUNC=$(printf "0x%x" "$func_num")
return
fi
elif [[ $addr =~ ^([0-9A-Fa-f]{2}):([0-9A-Fa-f]{2})\.([0-9A-Fa-f]{1})$
]]; then
bus_num=$((16#${BASH_REMATCH[1]}))
slot_num=$((16#${BASH_REMATCH[2]}))
func_num=$((16#${BASH_REMATCH[3]}))
if (( bus_num <= 0xff && slot_num <= 0xff && func_num <= 7 ));
then
DOMAIN="0x0000"
BUS=$(printf "0x%02x" "$bus_num")
SLOT=$(printf "0x%02x" "$slot_num")
FUNC=$(printf "0x%x" "$func_num")
return
fi
fi
DOMAIN="0x0000"
BUS="0x00"
SLOT="0x00"
FUNC="0x0"
```
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -506,10 +541,58 @@ 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"
- 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
+ DOMAIN=$(printf "0x%04x" "0x${BASH_REMATCH[1]}")
+ BUS=$(printf "0x%02x" "0x${BASH_REMATCH[2]}")
+ SLOT=$(printf "0x%02x" "0x${BASH_REMATCH[3]}")
+ FUNC=$(printf "0x%x" "0x${BASH_REMATCH[4]}")
+ elif [[ $addr =~ ^([0-9A-Fa-f]+):([0-9A-Fa-f]+)\.([0-9A-Fa-f]+)$ ]];
then
+ DOMAIN="0x0000"
+ BUS=$(printf "0x%02x" "0x${BASH_REMATCH[1]}")
+ SLOT=$(printf "0x%02x" "0x${BASH_REMATCH[2]}")
+ FUNC=$(printf "0x%x" "0x${BASH_REMATCH[3]}")
+ else
+ DOMAIN="0x0000"
+ BUS="0x00"
+ SLOT="0x00"
+ FUNC="0x0"
+ fi
+}
Review Comment:
On parse failure, `parse_pci_address` silently sets DOMAIN/BUS/SLOT/FUNC to
all zeros. That can produce incorrect libvirt addresses and misleading JSON
output while masking the underlying parsing problem. Consider returning a
non-zero status on failure (and letting callers handle it), or at minimum
logging/propagating an error and avoiding emitting a fabricated `0000:00:00.0`
address.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtGpuDef.java:
##########
@@ -79,28 +79,47 @@ 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();
- }
+ try {
+ String slotFunction;
+ 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];
+ } else {
+ throw new IllegalArgumentException("Invalid PCI bus
address format: '" + busAddress + "'");
}
+ String[] sf = slotFunction.split("\\.");
+ if (sf.length == 2) {
+ slotVal = Integer.parseInt(sf[0], 16);
+ funcVal = Integer.parseInt(sf[1].trim(), 16);
+ } else {
+ throw new IllegalArgumentException("Invalid PCI bus
address format: '" + busAddress + "'");
+ }
+ } catch (NumberFormatException e) {
+ throw new IllegalArgumentException("Invalid PCI bus address
format: '" + busAddress + "'", e);
}
}
+ 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);
Review Comment:
The parsing accepts any hex integer sizes and does not validate PCI range
constraints. This can generate invalid libvirt XML (e.g., domain > `0xffff`,
bus/slot > `0xff`, function outside `0..7`) and `%04x/%02x` will not truncate
values that exceed the intended width. Consider validating ranges after parsing
and throwing `IllegalArgumentException` for out-of-range values; also consider
using `Long.parseLong(..., 16)` for the domain to robustly handle NVIDIA-style
8-hex-digit domains before enforcing the `0..0xffff` constraint.
--
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]
