Copilot commented on code in PR #14266:
URL: https://github.com/apache/cloudstack/pull/14266#discussion_r4135425545
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -431,14 +431,16 @@ 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" };
+ // --details adds a Type column so cdrom slots (hdc, hdd) can be
filtered out — they sort
+ // alphabetically ahead of virtio disks, so without this the selected
row is a cdrom, not
+ // an actual disk.
+ String[] domblkCmd = new String[] {
Script.getExecutableAbsolutePath("virsh"), "domblklist", "--domain", vmName,
"--details" };
Review Comment:
`virsh domblklist --details` exposes a **Type** column, but the awk filter
is currently checking `$2=="disk"`. With the standard `--details` output (`Type
Device Target Source`), `Type` is `$1`, so this filter will never match and can
yield empty output (breaking device selection). Update the awk program to
filter on the correct field (Type) and keep printing the Target field.
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java:
##########
@@ -655,10 +656,33 @@ public void
testGetDeviceToAttachDiskPassesUnquotedAwkProgram() throws Exception
invokeGetDeviceToAttachDisk("test-vm");
- String[] awkCmd = captured[0].get(captured[0].size() - 1);
+ String[] awkCmd = captured[0].get(1);
// The commands are executed without a shell, so the program must
carry no shell quotes.
Assert.assertEquals("awk", awkCmd[0]);
- Assert.assertEquals("{print $1}", awkCmd[1]);
+ Assert.assertEquals("$2==\"disk\"{print $3}", awkCmd[1]);
Review Comment:
This test locks in the same incorrect awk field index as the production
code. Once the awk program is corrected to filter on the actual `Type` column
from `domblklist --details`, update the assertion accordingly so the test
validates the intended behavior (filtering by `Type == "disk"`).
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -431,14 +431,16 @@ 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" };
+ // --details adds a Type column so cdrom slots (hdc, hdd) can be
filtered out — they sort
+ // alphabetically ahead of virtio disks, so without this the selected
row is a cdrom, not
+ // an actual disk.
+ String[] domblkCmd = new String[] {
Script.getExecutableAbsolutePath("virsh"), "domblklist", "--domain", vmName,
"--details" };
// 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);
+ String[] awkCmd = new String[] {
Script.getExecutableAbsolutePath("awk"), "$2==\"disk\"{print $3}" };
Review Comment:
`virsh domblklist --details` exposes a **Type** column, but the awk filter
is currently checking `$2=="disk"`. With the standard `--details` output (`Type
Device Target Source`), `Type` is `$1`, so this filter will never match and can
yield empty output (breaking device selection). Update the awk program to
filter on the correct field (Type) and keep printing the Target field.
--
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]
