address review (#12826): parse PCI addresses with an XML parser, split slot selection into helpers, add unit tests

- getUsedPciSlots() parses the domain XML with the safer DocumentBuilderFactory
  and only considers <address type='pci'> elements, replacing the regex.
- getHighestNicSlot() and getFirstFreeSlotAbove() are separate methods.
- The javadoc now states the guarantee precisely: deterministic and monotonic
  after the last NIC, not contiguous when other devices sit in between.
- LibvirtPlugNicCommandWrapperTest covers parsing, selection and the fallbacks.

Signed-off-by: James Peru <jmsperu@gmail.com>

Author: jmsperu

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapper.java

@@ -30,19 +30,29 @@
 import com.cloud.resource.CommandWrapper;
 import com.cloud.resource.ResourceWrapper;
 import com.cloud.vm.VirtualMachine;
+import org.apache.cloudstack.utils.security.ParserUtils;
 import org.libvirt.Connect;
 import org.libvirt.Domain;
 import org.libvirt.LibvirtException;
-
+import org.w3c.dom.Document;
+import org.w3c.dom.Element;
+import org.w3c.dom.NodeList;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
+
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.ParserConfigurationException;
+import java.io.IOException;
+import java.io.StringReader;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 
 @ResourceWrapper(handles =  PlugNicCommand.class)
 public final class LibvirtPlugNicCommandWrapper extends CommandWrapper<PlugNicCommand, Answer, LibvirtComputingResource> {
 
+    /** Highest PCI slot number on a bus (0x00 is the host bridge). */
+    private static final int MAX_PCI_SLOT = 0x1f;
 
     @Override
     public Answer execute(final PlugNicCommand command, final LibvirtComputingResource libvirtComputingResource) {
@@ -70,10 +80,8 @@ public Answer execute(final PlugNicCommand command, final LibvirtComputingResour
                 libvirtComputingResource.setInterfaceDefQueueSettings(command.getDetails(), null, interfaceDef);
             }
 
-            // Explicitly assign PCI slot to ensure sequential NIC naming in the guest.
-            // Without this, libvirt auto-assigns the next free PCI slot which may be
-            // non-sequential with existing NICs (e.g. ens9 instead of ens5), causing
-            // guest network configuration to fail.
+            // Pin the PCI slot to the lowest free one above the existing NICs so the guest sees a
+            // deterministic, monotonic NIC order across hot-plugs (see findNextAvailablePciSlot).
             Integer nextSlot = findNextAvailablePciSlot(vm, pluggedNics);
             if (nextSlot != null) {
                 interfaceDef.setSlot(nextSlot);
@@ -113,52 +121,84 @@ public Answer execute(final PlugNicCommand command, final LibvirtComputingResour
     }
 
     /**
-     * Finds the next available PCI slot for a hot-plugged NIC by examining
-     * all PCI slots currently in use by the domain. This ensures the new NIC
-     * gets a sequential PCI address relative to existing NICs, resulting in
-     * predictable interface naming in the guest OS (e.g. ens5 instead of ens9).
+     * Picks the PCI slot for the NIC being hot-plugged: the lowest free slot above the highest slot
+     * already used by a NIC. The choice is deterministic and monotonic with respect to the NICs that
+     * are already present (a new NIC never lands below an existing one), which is what keeps the
+     * guest's predictable interface names stable across hot-plugs. It does not guarantee contiguity:
+     * slots between the last NIC and the new one may already be taken by other devices (disks,
+     * controllers, balloon), in which case the next free slot above them is used.
+     *
+     * @return the slot to assign, or {@code null} to let libvirt auto-assign (domain XML unavailable
+     *         or unparseable, or no free slot left).
      */
-    private Integer findNextAvailablePciSlot(final Domain vm, final List<InterfaceDef> pluggedNics) {
+    protected Integer findNextAvailablePciSlot(final Domain vm, final List<InterfaceDef> pluggedNics) {
         try {
-            String domXml = vm.getXMLDesc(0);
-
-            // Defensive: getXMLDesc can return null on certain libvirt error paths (and is
-            // null in unit tests where the Domain mock isn't stubbed for this call). Fall
-            // back to letting libvirt auto-assign the PCI slot.
+            final String domXml = vm.getXMLDesc(0);
+            // getXMLDesc can return null on certain libvirt error paths; fall back to libvirt's own choice.
             if (domXml == null) {
                 logger.debug("Domain XML unavailable, letting libvirt auto-assign PCI slot");
                 return null;
             }
-
-            // Parse all PCI slot numbers currently in use
-            Set<Integer> usedSlots = new HashSet<>();
-            Pattern slotPattern = Pattern.compile("slot='0x([0-9a-fA-F]+)'");
-            Matcher matcher = slotPattern.matcher(domXml);
-            while (matcher.find()) {
-                usedSlots.add(Integer.parseInt(matcher.group(1), 16));
+            final Set<Integer> usedSlots = getUsedPciSlots(domXml);
+            if (usedSlots == null) {
+                return null;
             }
+            final Integer slot = getFirstFreeSlotAbove(getHighestNicSlot(pluggedNics), usedSlots);
+            if (slot == null) {
+                logger.warn("No free PCI slots available, letting libvirt auto-assign");
+            }
+            return slot;
+        } catch (final LibvirtException e) {
+            logger.warn("Failed to get domain XML for PCI slot calculation, letting libvirt auto-assign", e);
+            return null;
+        }
+    }
 
-            // Find the highest PCI slot used by existing NICs
-            int maxNicSlot = 0;
-            for (InterfaceDef pluggedNic : pluggedNics) {
-                if (pluggedNic.getSlot() != null && pluggedNic.getSlot() > maxNicSlot) {
-                    maxNicSlot = pluggedNic.getSlot();
+    /**
+     * Collects the slot numbers of every {@code <address type='pci' .../>} element in the domain XML,
+     * whichever device or bus they belong to. Returns {@code null} if the XML cannot be parsed.
+     */
+    protected Set<Integer> getUsedPciSlots(final String domXml) {
+        final Set<Integer> usedSlots = new HashSet<>();
+        try {
+            final DocumentBuilder builder = ParserUtils.getSaferDocumentBuilderFactory().newDocumentBuilder();
+            final Document doc = builder.parse(new InputSource(new StringReader(domXml)));
+            final NodeList addresses = doc.getElementsByTagName("address");
+            for (int i = 0; i < addresses.getLength(); i++) {
+                final Element address = (Element) addresses.item(i);
+                if (!"pci".equals(address.getAttribute("type")) || address.getAttribute("slot").isEmpty()) {
+                    continue;
                 }
+                usedSlots.add(Integer.decode(address.getAttribute("slot")));
             }
+        } catch (final ParserConfigurationException | SAXException | IOException | NumberFormatException e) {
+            logger.warn("Failed to parse domain XML for PCI slot calculation, letting libvirt auto-assign", e);
+            return null;
+        }
+        return usedSlots;
+    }
 
-            // Find next free slot starting from maxNicSlot + 1
-            // PCI slots range from 0x01 to 0x1f (slot 0 is reserved for host bridge)
-            for (int slot = maxNicSlot + 1; slot <= 0x1f; slot++) {
-                if (!usedSlots.contains(slot)) {
-                    return slot;
-                }
+    /** Highest PCI slot used by an existing NIC, or 0 when no NIC carries a slot. */
+    protected static int getHighestNicSlot(final List<InterfaceDef> pluggedNics) {
+        int highest = 0;
+        for (final InterfaceDef pluggedNic : pluggedNics) {
+            if (pluggedNic.getSlot() != null && pluggedNic.getSlot() > highest) {
+                highest = pluggedNic.getSlot();
             }
+        }
+        return highest;
+    }
 
-            logger.warn("No free PCI slots available, letting libvirt auto-assign");
-            return null;
-        } catch (LibvirtException e) {
-            logger.warn("Failed to get domain XML for PCI slot calculation, letting libvirt auto-assign", e);
-            return null;
+    /**
+     * Lowest slot strictly above {@code from} (and no higher than {@link #MAX_PCI_SLOT}) that is not in
+     * {@code usedSlots}; {@code null} when the range is exhausted. Slot 0 is reserved for the host bridge.
+     */
+    protected static Integer getFirstFreeSlotAbove(final int from, final Set<Integer> usedSlots) {
+        for (int slot = from + 1; slot <= MAX_PCI_SLOT; slot++) {
+            if (!usedSlots.contains(slot)) {
+                return slot;
+            }
         }
+        return null;
     }
 }

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapperTest.java

@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.libvirt.Domain;
+import org.libvirt.LibvirtException;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.InterfaceDef;
+
+@RunWith(MockitoJUnitRunner.class)
+public class LibvirtPlugNicCommandWrapperTest {
+
+    /**
+     * Typical q35-less layout: NICs at 0x03 and 0x06, other PCI devices at 0x01, 0x02, 0x04, 0x05 and 0x07,
+     * plus non-PCI addresses (drive, usb) that must be ignored.
+     */
+    private static final String DOMAIN_XML =
+            "<domain type='kvm'>\n"
+            + "  <name>i-2-42-VM</name>\n"
+            + "  <devices>\n"
+            + "    <disk type='file' device='disk'>\n"
+            + "      <target dev='vda' bus='virtio'/>\n"
+            + "      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>\n"
+            + "    </disk>\n"
+            + "    <disk type='file' device='cdrom'>\n"
+            + "      <target dev='hdc' bus='ide'/>\n"
+            + "      <address type='drive' controller='0' bus='1' target='0' unit='0'/>\n"
+            + "    </disk>\n"
+            + "    <controller type='usb' index='0'>\n"
+            + "      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>\n"
+            + "    </controller>\n"
+            + "    <controller type='virtio-serial' index='0'>\n"
+            + "      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>\n"
+            + "    </controller>\n"
+            + "    <interface type='bridge'>\n"
+            + "      <mac address='02:00:7c:98:00:01'/>\n"
+            + "      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>\n"
+            + "    </interface>\n"
+            + "    <interface type='bridge'>\n"
+            + "      <mac address='02:00:7c:98:00:02'/>\n"
+            + "      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>\n"
+            + "    </interface>\n"
+            + "    <channel type='unix'>\n"
+            + "      <address type='virtio-serial' controller='0' bus='0' port='1'/>\n"
+            + "    </channel>\n"
+            + "    <video>\n"
+            + "      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>\n"
+            + "    </video>\n"
+            + "    <memballoon model='virtio'>\n"
+            + "      <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/>\n"
+            + "    </memballoon>\n"
+            + "  </devices>\n"
+            + "</domain>\n";
+
+    @Mock
+    private Domain domain;
+
+    private final LibvirtPlugNicCommandWrapper wrapper = new LibvirtPlugNicCommandWrapper();
+
+    private static InterfaceDef nicAtSlot(final Integer slot) {
+        final InterfaceDef nic = new InterfaceDef();
+        nic.setSlot(slot);
+        return nic;
+    }
+
+    private static List<InterfaceDef> nicsFromXml() {
+        return Arrays.asList(nicAtSlot(0x03), nicAtSlot(0x06));
+    }
+
+    @Test
+    public void getUsedPciSlotsOnlyCountsPciAddresses() {
+        final Set<Integer> used = wrapper.getUsedPciSlots(DOMAIN_XML);
+        assertEquals(Set.of(0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07), used);
+    }
+
+    @Test
+    public void getUsedPciSlotsReturnsNullOnMalformedXml() {
+        assertNull(wrapper.getUsedPciSlots("<domain><devices><interface>"));
+    }
+
+    @Test
+    public void getHighestNicSlotIgnoresNicsWithoutAddress() {
+        assertEquals(0x06, LibvirtPlugNicCommandWrapper.getHighestNicSlot(Arrays.asList(nicAtSlot(0x03), nicAtSlot(null), nicAtSlot(0x06))));
+        assertEquals(0, LibvirtPlugNicCommandWrapper.getHighestNicSlot(Collections.emptyList()));
+    }
+
+    @Test
+    public void getFirstFreeSlotAboveSkipsOccupiedSlotsAndStopsAtBusEnd() {
+        assertEquals(Integer.valueOf(0x08), LibvirtPlugNicCommandWrapper.getFirstFreeSlotAbove(0x06, Set.of(0x07)));
+        assertEquals(Integer.valueOf(0x07), LibvirtPlugNicCommandWrapper.getFirstFreeSlotAbove(0x06, Collections.emptySet()));
+        assertNull(LibvirtPlugNicCommandWrapper.getFirstFreeSlotAbove(0x1f, Collections.emptySet()));
+    }
+
+    @Test
+    public void findNextAvailablePciSlotPicksLowestFreeSlotAboveLastNic() throws LibvirtException {
+        when(domain.getXMLDesc(0)).thenReturn(DOMAIN_XML);
+        // 0x07 is taken by the balloon, so the NIC goes to 0x08: monotonic after the last NIC, not contiguous.
+        assertEquals(Integer.valueOf(0x08), wrapper.findNextAvailablePciSlot(domain, nicsFromXml()));
+    }
+
+    @Test
+    public void findNextAvailablePciSlotFallsBackWhenXmlUnavailable() throws LibvirtException {
+        when(domain.getXMLDesc(0)).thenReturn(null);
+        assertNull(wrapper.findNextAvailablePciSlot(domain, nicsFromXml()));
+    }
+
+    @Test
+    public void findNextAvailablePciSlotFallsBackWhenLibvirtFails() throws LibvirtException {
+        when(domain.getXMLDesc(0)).thenThrow(Mockito.mock(LibvirtException.class));
+        assertNull(wrapper.findNextAvailablePciSlot(domain, nicsFromXml()));
+    }
+
+    @Test
+    public void findNextAvailablePciSlotFallsBackWhenBusIsFull() throws LibvirtException {
+        when(domain.getXMLDesc(0)).thenReturn(DOMAIN_XML);
+        assertNull(wrapper.findNextAvailablePciSlot(domain, Collections.singletonList(nicAtSlot(0x1f))));
+    }
+}