Copilot commented on code in PR #14266:
URL: https://github.com/apache/cloudstack/pull/14266#discussion_r4281746334
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -431,23 +433,30 @@ private boolean attachRbdVolumeToVm(KVMStoragePoolManager
storagePoolMgr, String
}
private String getDeviceToAttachDisk(String vmName) {
- String[] domblkCmd = new String[] {
Script.getExecutableAbsolutePath("virsh"), "domblklist", "--domain", vmName };
- String[] tailCmd = new String[] {
Script.getExecutableAbsolutePath("tail"), "-n", "3" };
- String[] headCmd = new String[] {
Script.getExecutableAbsolutePath("head"), "-n", "1" };
- // The commands are executed without a shell, so the awk program must
be passed as a plain
- // argument. Keeping the quotes a shell would have stripped makes awk
fail with
- // "invalid char" and produce no output.
- String[] awkCmd = new String[] {
Script.getExecutableAbsolutePath("awk"), "{print $1}" };
- Pair<Integer, String> result =
Script.executePipedCommands(Arrays.asList(domblkCmd, tailCmd, headCmd, awkCmd),
0);
- // executePipedCommands appends a line separator to every line it
reads, so the device
- // name has to be trimmed before the last character can be incremented.
- String currentDevice = result.second() == null ? "" :
result.second().trim();
- if (result.first() == null || result.first() != 0 ||
StringUtils.isBlank(currentDevice)) {
- throw new CloudRuntimeException(String.format("Failed to determine
the device to attach the restored volume to on VM [%s].", vmName));
+ // virsh runs on its own so its exit code is checked; behind a pipe
only the last command's is.
+ // --details adds the Device column (disk/cdrom) used below to skip
the cdrom slots.
+ String[] domblkCmd = new String[] {
Script.getExecutableAbsolutePath("virsh"), "domblklist", "--domain", vmName,
"--details" };
+ Pair<Integer, String> result =
Script.executePipedCommands(Collections.singletonList(domblkCmd), 0);
+ if (result.first() == null || result.first() != 0 || result.second()
== null) {
+ throw new CloudRuntimeException(String.format("Failed to list the
disks of VM [%s] to determine the device to attach the restored volume to.",
vmName));
}
- char lastChar = currentDevice.charAt(currentDevice.length() - 1);
- char incrementedChar = (char) (lastChar + 1);
- return currentDevice.substring(0, currentDevice.length() - 1) +
incrementedChar;
+ // Rows are "<Type> <Device> <Target> <Source>". Only virtio (vd*)
targets are collected: IDE/SATA
+ // disks cannot be hot-plugged. domblklist follows the domain XML
order, where hot-plugged disks
+ // are appended, so the last target is not necessarily the highest
one; pick the first free name.
+ Set<String> usedDevices = new HashSet<>();
+ for (String line : result.second().split("\\R")) {
+ String[] columns = line.trim().split("\\s+");
+ if (columns.length >= 3 && "disk".equals(columns[1]) &&
columns[2].startsWith("vd")) {
+ usedDevices.add(columns[2]);
+ }
+ }
+ for (char c = 'a'; c <= 'z'; c++) {
+ String device = "vd" + c;
+ if (!usedDevices.contains(device)) {
+ return device;
+ }
+ }
+ throw new CloudRuntimeException(String.format("No free virtio device
is left to attach the restored volume to on VM [%s].", vmName));
Review Comment:
This limits attachable virtio targets to vda–vdz. Libvirt/QEMU device naming
continues with multi-letter suffixes (e.g., vdaa, vdab, ...), so VMs with >26
virtio disks would incorrectly fail to attach. Consider generating `vd` names
with a base-26 suffix (a..z, aa..az, ...) and scanning for the first unused
value, or converting existing suffixes to an index and iterating indices until
a free one is found.
--
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]
