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

2023-10-19 Thread 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 
> >---
> >  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 00..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
> >+ * .
> >+ */
> >+
> >+#include 
> >+
> >+#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

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

2023-10-18 Thread Laine Stump 

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 
---
  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 00..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
+ * .
+ */
+
+#include 
+
+#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

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

2023-10-16 Thread Praveen K Paladugu 
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 
---
 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 00..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
+ * .
+ */
+
+#include 
+
+#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)
+{
+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");
+if (virDomainInterfaceIsVnetCompatModel(net))
+tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
+
+if (net->managed_tap == VIR_TRISTATE_BOOL_NO) {
+if (!net->ifname) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("target dev must be supplied when managed='no'"));
+goto cleanup;
+}
+if (virNetDevExists(net->ifname) != 1) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("target managed='no' but specified dev doesn't 
exist"));
+goto cleanup;
+}
+
+