Copilot commented on code in PR #14266:
URL: https://github.com/apache/cloudstack/pull/14266#discussion_r4247806633
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -431,14 +431,17 @@ 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 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:
These lines conflict with the PR description, which states CD-ROM targets
sort alphabetically ahead of virtio disks. To keep the rationale consistent and
avoid encoding a potentially incorrect ordering assumption, consider rewording
to focus on the core issue (CD-ROM rows may appear in the output, and must be
excluded via `--details` + filtering) without claiming a specific ordering.
##########
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]);
+ }
+ }
+
+ @Test
+ public void testGetDeviceToAttachDiskRequestsDomblklistDetails() throws
Exception {
+ try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
+ scriptMock.when(() ->
Script.getExecutableAbsolutePath(anyString()))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+ final AtomicReference<List<String[]>> captured = new
AtomicReference<>();
+ scriptMock.when(() -> Script.executePipedCommands(anyList(),
anyLong()))
+ .thenAnswer(invocation -> {
+ captured.set(invocation.getArgument(0));
+ return new Pair<>(0, "vdb" + System.lineSeparator());
+ });
+
+ Assert.assertEquals("vdc", invokeGetDeviceToAttachDisk("test-vm"));
+
+ // --details adds the Type column the awk program filters on
(verified separately in
+ // testGetDeviceToAttachDiskPassesUnquotedAwkProgram); without it
cdrom rows can't be
+ // told apart from real disks at all.
+ String[] domblkCmd = captured.get().get(0);
+ Assert.assertTrue("domblklist must request --details so the Type
column is available to filter on",
Review Comment:
This comment is inaccurate: the awk filter in the corresponding production
code checks `$2==\"disk\"`, which refers to the `Device` column (disk/cdrom),
not the `Type` column. Please update the comment to refer to the correct column
to avoid misleading future maintainers.
--
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]
