Copilot commented on code in PR #14266:
URL: https://github.com/apache/cloudstack/pull/14266#discussion_r4134116606
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java:
##########
@@ -655,10 +655,30 @@ 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]);
+ }
+ }
+
+ @Test
+ public void testGetDeviceToAttachDiskFiltersOutCdromEntries() throws
Exception {
+ try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
+ scriptMock.when(() ->
Script.getExecutableAbsolutePath(anyString()))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+ final List<String[]>[] captured = new List[1];
+ scriptMock.when(() -> Script.executePipedCommands(anyList(),
anyLong()))
+ .thenAnswer(invocation -> {
+ captured[0] = invocation.getArgument(0);
+ return new Pair<>(0, "vdb" + System.lineSeparator());
+ });
+
+ Assert.assertEquals("vdc", invokeGetDeviceToAttachDisk("test-vm"));
+
+ String[] domblkCmd = captured[0].get(0);
+ Assert.assertTrue("domblklist must request --details so the Type
column is available to filter on",
+ Arrays.asList(domblkCmd).contains("--details"));
Review Comment:
This test name suggests it verifies CD-ROM entries are filtered out, but the
assertions only check that `--details` was included in the `domblklist` command
(the filtering behavior is actually covered by the awk-program assertion in the
other test). Either rename this test to reflect what it asserts (e.g.,
`...RequestsDomblklistDetails`) or extend it to explicitly validate the
filtering behavior being applied.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -431,14 +431,15 @@ 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
+ // after the real disks, so without this the last row is always a
cdrom, not a free device.
+ String[] domblkCmd = new String[] {
Script.getExecutableAbsolutePath("virsh"), "domblklist", "--domain", vmName,
"--details" };
Review Comment:
The comment claims CD-ROM slots “sort after the real disks”, but the PR
description/root cause states they sort alphabetically ahead of virtio disks.
Please align the comment with the actual observed ordering and the real failure
mode (i.e., that CD-ROM entries must be excluded from consideration regardless
of position) so future readers don’t get a contradictory explanation.
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java:
##########
@@ -655,10 +655,30 @@ 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]);
+ }
+ }
+
+ @Test
+ public void testGetDeviceToAttachDiskFiltersOutCdromEntries() throws
Exception {
+ try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
+ scriptMock.when(() ->
Script.getExecutableAbsolutePath(anyString()))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+ final List<String[]>[] captured = new List[1];
Review Comment:
Using an array-of-generic (`new List[1]`) as a mutable holder is not
type-safe and typically triggers unchecked warnings. Consider replacing it with
an `AtomicReference<List<String[]>>` (or similar typed holder) to avoid
compiler warnings and make the intent clearer.
--
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]
