On Wed, Oct 18, 2023 at 03:36:47PM -0400, Laine Stump wrote: > 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@xxxxxxxxxxxxxxxxxxx> > >--- > > 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 started with enabling the "ethernet" mode, so I moved all the related methods. I am planning to also enable "Bridge" and "NAT" modes too in follow up patches. I did this to test the methods properly before I push. > > 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. Thanks for the pointer here. I will refer to this while enabling the additional network modes. > > 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. Understood. I agree it would be best to keep these methods together. I will push an updated version which also moves qemuInterfaceStopDevices(). > > (as for qemuInterfacePrepareSlirp() and qemuInterfaceOpenVhostNet(), > those two functions are clearly completely QEMU-specific and so > should remain in qemu_interface.c) Understood. > > > >+{ > >+ 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. Ahh.. yes. I will drop this.
