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

[Praveen Paladugu]

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 at linux.microsoft.com>
> >---
> >  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.