Copilot commented on code in PR #14266:
URL: https://github.com/apache/cloudstack/pull/14266#discussion_r4280691461
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -431,19 +431,24 @@ 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);
+ // --details adds a Device column (disk/cdrom) that the awk below
filters on ($2=="disk")
+ // to exclude cdrom slots. cdrom entries (hdc, hdd) are listed after
the virtio disks in
+ // domblklist's output, so without this filter, taking the last line
would select a
+ // cdrom, not the actual disk.
+ String[] domblkCmd = new String[] {
Script.getExecutableAbsolutePath("virsh"), "domblklist", "--domain", vmName,
"--details" };
+ // Only virtio (vd*) targets are used: IDE/SATA disks cannot be
hot-plugged.
+ String[] awkCmd = new String[] {
Script.getExecutableAbsolutePath("awk"), "$2==\"disk\" && $3 ~ /^vd/ {print
$3}" };
+ String[] tailCmd = new String[] {
Script.getExecutableAbsolutePath("tail"), "-n", "1" };
+ Pair<Integer, String> result =
Script.executePipedCommands(Arrays.asList(domblkCmd, awkCmd, tailCmd), 0);
Review Comment:
Spawning a separate `tail` process is avoidable here; you can have awk print
only the last matching target (e.g., store `$3` per match and print it in an
`END` block). This reduces process overhead and simplifies the pipeline while
keeping the same behavior.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -431,19 +431,24 @@ 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);
+ // --details adds a Device column (disk/cdrom) that the awk below
filters on ($2=="disk")
+ // to exclude cdrom slots. cdrom entries (hdc, hdd) are listed after
the virtio disks in
+ // domblklist's output, so without this filter, taking the last line
would select a
+ // cdrom, not the actual disk.
Review Comment:
The comment about CD-ROM entries being “listed after the virtio disks”
appears inconsistent with the PR description (CD-ROM targets like `hdc`/`hdd`
sort alphabetically ahead of `vd*`). This is misleading for future maintainers;
please update the comment to describe the actual ordering issue (or avoid
stating an ordering guarantee and focus on “filter out cdrom rows” as the core
requirement).
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -431,19 +431,24 @@ 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);
+ // --details adds a Device column (disk/cdrom) that the awk below
filters on ($2=="disk")
+ // to exclude cdrom slots. cdrom entries (hdc, hdd) are listed after
the virtio disks in
+ // domblklist's output, so without this filter, taking the last line
would select a
+ // cdrom, not the actual disk.
+ String[] domblkCmd = new String[] {
Script.getExecutableAbsolutePath("virsh"), "domblklist", "--domain", vmName,
"--details" };
+ // Only virtio (vd*) targets are used: IDE/SATA disks cannot be
hot-plugged.
+ String[] awkCmd = new String[] {
Script.getExecutableAbsolutePath("awk"), "$2==\"disk\" && $3 ~ /^vd/ {print
$3}" };
+ String[] tailCmd = new String[] {
Script.getExecutableAbsolutePath("tail"), "-n", "1" };
+ Pair<Integer, String> result =
Script.executePipedCommands(Arrays.asList(domblkCmd, awkCmd, tailCmd), 0);
+ if (result.first() == null || result.first() != 0) {
+ throw new CloudRuntimeException(String.format("Failed to determine
the device to attach the restored volume to on VM [%s].", vmName));
+ }
// 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));
+ if (StringUtils.isBlank(currentDevice)) {
+ // The VM has no virtio disk yet (e.g. only an IDE or SCSI root
disk), so vda is free.
+ return "vda";
}
char lastChar = currentDevice.charAt(currentDevice.length() - 1);
char incrementedChar = (char) (lastChar + 1);
Review Comment:
Incrementing only the last character of the target breaks for boundary cases
like `vdz` (would become `{`) and doesn’t handle multi-letter suffixes (`vdaa`,
etc.). It can also skip free names if there are gaps (e.g., existing `vda` and
`vdc` will choose `vdd` even though `vdb` is free). A more robust approach is
to parse all existing `vd*` targets into a set and then pick the first free
name from `vda` upward (including rollover past `vdz`), rather than
incrementing the last character.
--
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]
