Re: [libvirt PATCH v2] hypervisor: Move interface mgmt methods to hypervisor

On 10/16/23 3:34 PM, Praveen K Paladugu wrote: >Move guest interface management methods from qemu to hypervisor. These >methods will be shared by networking support in ch driver. > >Signed-off-by: Praveen K Paladugu <prapal(a)linux.microsoft.com&gt; >--- > po/POTFILES | 1 + > src/hypervisor/domain_interface.c | 280 ++++++++++++++++++++++++++++++ > src/hypervisor/domain_interface.h | 39 +++++ > src/hypervisor/meson.build | 1 + > src/libvirt_private.syms | 6 + > src/qemu/qemu_command.c | 7 +- > src/qemu/qemu_hotplug.c | 3 +- > src/qemu/qemu_interface.c | 246 +------------------------- > src/qemu/qemu_interface.h | 6 - > src/qemu/qemu_process.c | 3 +- > 10 files changed, 343 insertions(+), 249 deletions(-) > create mode 100644 src/hypervisor/domain_interface.c > create mode 100644 src/hypervisor/domain_interface.h > >diff --git a/po/POTFILES b/po/POTFILES >index 3a51aea5cb..023c041f61 100644 >--- a/po/POTFILES >+++ b/po/POTFILES >@@ -92,6 +92,7 @@ src/hyperv/hyperv_util.c > src/hyperv/hyperv_wmi.c > src/hypervisor/domain_cgroup.c > src/hypervisor/domain_driver.c >+src/hypervisor/domain_interface.c > src/hypervisor/virhostdev.c > src/interface/interface_backend_netcf.c > src/interface/interface_backend_udev.c >diff --git a/src/hypervisor/domain_interface.c b/src/hypervisor/domain_interface.c >new file mode 100644 >index 0000000000..592c4253df >--- /dev/null >+++ b/src/hypervisor/domain_interface.c >@@ -0,0 +1,280 @@ >+/* >+ * Copyright (C) 2015-2016 Red Hat, Inc. >+ * Copyright IBM Corp. 2014 >+ * >+ * domain_interface.c: methods to manage guest/domain interfaces >+ * >+ * This library is free software; you can redistribute it and/or >+ * modify it under the terms of the GNU Lesser General Public >+ * License as published by the Free Software Foundation; either >+ * version 2.1 of the License, or (at your option) any later version. >+ * >+ * This library is distributed in the hope that it will be useful, >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >+ * Lesser General Public License for more details. >+ * >+ * You should have received a copy of the GNU Lesser General Public >+ * License along with this library. If not, see >+ * <http://www.gnu.org/licenses/>;. >+ */ >+ >+#include <config.h> >+ >+#include "virconftypes.h" >+#include "virmacaddr.h" >+#include "virnetdevtap.h" >+#include "domain_audit.h" >+#include "domain_conf.h" >+#include "domain_interface.h" >+#include "domain_nwfilter.h" >+#include "viralloc.h" >+#include "virebtables.h" >+#include "virfile.h" >+#include "virlog.h" >+#include "virnetdevbridge.h" >+#include "network_conf.h" >+ >+#define VIR_FROM_THIS VIR_FROM_INTERFACE >+ >+VIR_LOG_INIT("interface.interface_connect"); >+ >+bool >+virDomainInterfaceIsVnetCompatModel(const virDomainNetDef *net) >+{ >+ return (virDomainNetIsVirtioModel(net) || >+ net->model == VIR_DOMAIN_NET_MODEL_E1000E || >+ net->model == VIR_DOMAIN_NET_MODEL_IGB || >+ net->model == VIR_DOMAIN_NET_MODEL_VMXNET3); >+} >+ >+/* virDomainInterfaceEthernetConnect: >+ * @def: the definition of the VM >+ * @driver: qemu driver data >+ * @net: pointer to the VM's interface description >+ * @tapfd: array of file descriptor return value for the new device >+ * @tapfdsize: number of file descriptors in @tapfd >+ * >+ * Called *only* called if actualType is VIR_DOMAIN_NET_TYPE_ETHERNET >+ * (i.e. if the connection is made with a tap device) >+ */ >+int >+virDomainInterfaceEthernetConnect(virDomainDef *def, >+ virDomainNetDef *net, >+ ebtablesContext *ebtables, >+ bool macFilter, >+ bool privileged, >+ int *tapfd, >+ size_t tapfdSize) This is all fine as far as it goes, but I was expecting that all of the functions at the same level (qemuInterface*Connect()) would be genericized and moved to the new file. I guess you're only planning to use the plain "ethernet" type of interface in the ch driver, but for QEMU to have some of the *Connect functions in one place and some in another makes the code more confusing; this is especially so since virDomainInterfaceStartDevice() has cases for these other interface types.

I'm guessing possibly one of the reasons for not moving the Connect() functions for the other types over was because they are using QEMU-specific datatypes and functions that can't be called from the hypervisor directory (because files there can only call to functions in src/util, src/conf, and src/hypervisor). But that can be solved fairly easily since the QEMU-specific stuff is just some strings in the driver config - you can just modify the QEMU caller of that code to retrieve the string from the QEMU config and pass it to the Connect() function as a string.

Along with this, it would be more cohesive if qemuInterfaceStopDevices() was moved into the new file along with qemuInterfaceStartDevices() (which you're already moving). Again, I guess you didn't do that because you're only planning to use TYPE_ETHERNET, and the case for that type is a NOP in qemuInterfaceStartDevices(). Still, splitting the functionality in two like that will make life confusing for anyone trying to understand the code.

(as for qemuInterfacePrepareSlirp() and qemuInterfaceOpenVhostNet(), those two functions are clearly completely QEMU-specific and so should remain in qemu_interface.c)

>+{ >+ virMacAddr tapmac; >+ int ret = -1; >+ unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP; >+ bool template_ifname = false; >+ const char *tunpath = "/dev/net/tun"; >+ const char *auditdev = tunpath; >+ >+ if (net->backend.tap) { >+ tunpath = net->backend.tap; >+ if (!privileged) { >+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >+ _("cannot use custom tap device in session mode")); >+ goto cleanup; >+ } >+ } >+ VIR_WARN("PPK: interfaceEthernetConnect Called\n"); Looks like you added a log message for debug and forgot to remove it.