[PATCH 0/2] Enable NAT network support for ch guests

Move additional domain interface management methods to hypervisor and enable NAT mode network support for ch guests. Praveen K Paladugu (2): hypervisor: Move domain interface mgmt methods ch: Enable NAT Network mode support src/ch/ch_interface.c | 57 ++++++-- src/hypervisor/domain_interface.c | 228 ++++++++++++++++++++++++++++++ src/hypervisor/domain_interface.h | 10 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 9 +- src/qemu/qemu_interface.c | 219 +--------------------------- 6 files changed, 289 insertions(+), 235 deletions(-) -- 2.44.0

From: Praveen K Paladugu <praveenkpaladugu@gmail.com> From: Praveen K Paladugu <prapal@linux.microsoft.com> Move methods to connect domain interfaces to host bridges to hypervisor. This is to allow reuse between qemu and ch drivers. Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/hypervisor/domain_interface.c | 228 ++++++++++++++++++++++++++++++ src/hypervisor/domain_interface.h | 10 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 9 +- src/qemu/qemu_interface.c | 219 +--------------------------- 5 files changed, 247 insertions(+), 220 deletions(-) diff --git a/src/hypervisor/domain_interface.c b/src/hypervisor/domain_interface.c index 756abb08e9..0c19ff859a 100644 --- a/src/hypervisor/domain_interface.c +++ b/src/hypervisor/domain_interface.c @@ -39,6 +39,7 @@ #include "virnetdevmidonet.h" #include "virnetdevopenvswitch.h" #include "virnetdevtap.h" +#include "vircommand.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -514,3 +515,230 @@ virDomainClearNetBandwidth(virDomainDef *def) virDomainInterfaceClearQoS(def, def->nets[i]); } } + +/** + * virDomainCreateInBridgePortWithHelper: + * @bridgeHelperName: name of the bridge helper program + * @brname: the bridge name + * @ifname: the returned interface name + * @tapfd: file descriptor return value for the new tap device + * @flags: OR of virNetDevTapCreateFlags: + + * VIR_NETDEV_TAP_CREATE_VNET_HDR + * - Enable IFF_VNET_HDR on the tap device + * + * This function creates a new tap device on a bridge using an external + * helper. The final name for the bridge will be stored in @ifname. + * + * Returns 0 in case of success or -1 on failure + */ +static int +virDomainCreateInBridgePortWithHelper(const char *bridgeHelperName, + const char *brname, + char **ifname, + int *tapfd, + unsigned int flags) +{ + const char *const bridgeHelperDirs[] = { + "/usr/libexec", + "/usr/lib/qemu", + "/usr/lib", + NULL, + }; + g_autoptr(virCommand) cmd = NULL; + g_autofree char *bridgeHelperPath = NULL; + char *errbuf = NULL, *cmdstr = NULL; + int pair[2] = { -1, -1 }; + + if (!bridgeHelperName) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing bridge helper name")); + return -1; + } + + if ((flags & ~VIR_NETDEV_TAP_CREATE_VNET_HDR) != VIR_NETDEV_TAP_CREATE_IFUP) + return -1; + + if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) { + virReportSystemError(errno, "%s", _("failed to create socket")); + return -1; + } + + bridgeHelperPath = virFindFileInPathFull(bridgeHelperName, bridgeHelperDirs); + + if (!bridgeHelperPath) { + virReportSystemError(errno, _("'%1$s' is not a suitable bridge helper"), + bridgeHelperName); + return -1; + } + + VIR_DEBUG("Using qemu-bridge-helper: %s", bridgeHelperPath); + cmd = virCommandNew(bridgeHelperPath); + if (flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) + virCommandAddArgFormat(cmd, "--use-vnet"); + virCommandAddArgFormat(cmd, "--br=%s", brname); + virCommandAddArgFormat(cmd, "--fd=%d", pair[1]); + virCommandSetErrorBuffer(cmd, &errbuf); + virCommandDoAsyncIO(cmd); + virCommandPassFD(cmd, pair[1], + VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandClearCaps(cmd); +#ifdef CAP_NET_ADMIN + virCommandAllowCap(cmd, CAP_NET_ADMIN); +#endif + if (virCommandRunAsync(cmd, NULL) < 0) { + *tapfd = -1; + goto cleanup; + } + + do { + *tapfd = virSocketRecvFD(pair[0], 0); + } while (*tapfd < 0 && errno == EINTR); + + if (*tapfd < 0) { + char *errstr = NULL; + + if (!(cmdstr = virCommandToString(cmd, false))) + goto cleanup; + virCommandAbort(cmd); + + if (errbuf && *errbuf) + errstr = g_strdup_printf("stderr=%s", errbuf); + + virReportSystemError(errno, + _("%1$s: failed to communicate with bridge helper: %2$s"), + cmdstr, + NULLSTR_EMPTY(errstr)); + VIR_FREE(errstr); + goto cleanup; + } + + if (virNetDevTapGetName(*tapfd, ifname) < 0 || + virCommandWait(cmd, NULL) < 0) { + VIR_FORCE_CLOSE(*tapfd); + *tapfd = -1; + } + + cleanup: + VIR_FREE(cmdstr); + VIR_FREE(errbuf); + VIR_FORCE_CLOSE(pair[0]); + return *tapfd < 0 ? -1 : 0; +} + + +/* virDomainInterfaceBridgeConnect: + * @def: the definition of the VM + * @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 + * @privileged: whether running as privileged user + * @ebtables: ebtales context + * @macFilter: whether driver support mac Filtering + * @bridgeHelperName:name of the bridge helper program to run in non-privileged mode + * + * Called *only* called if actualType is VIR_DOMAIN_NET_TYPE_NETWORK or + * VIR_DOMAIN_NET_TYPE_BRIDGE (i.e. if the connection is made with a tap + * device connecting to a bridge device) + */ +int +virDomainInterfaceBridgeConnect(virDomainDef *def, + virDomainNetDef *net, + int *tapfd, + size_t tapfdSize, + bool privileged, + ebtablesContext *ebtables, + bool macFilter, + const char *bridgeHelperName) +{ + const char *brname; + int ret = -1; + unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP; + bool template_ifname = false; + const char *tunpath = "/dev/net/tun"; + + 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; + } + } + + if (!(brname = virDomainNetGetActualBridgeName(net))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing bridge name")); + goto cleanup; + } + + if (!net->ifname) + template_ifname = true; + + if (virDomainInterfaceIsVnetCompatModel(net)) + tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR; + + if (privileged) { + if (virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, + def->uuid, tunpath, tapfd, tapfdSize, + virDomainNetGetActualVirtPortProfile(net), + virDomainNetGetActualVlan(net), + virDomainNetGetActualPortOptionsIsolated(net), + net->coalesce, 0, NULL, + tap_create_flags) < 0) { + virDomainAuditNetDevice(def, net, tunpath, false); + goto cleanup; + } + if (virDomainNetGetActualBridgeMACTableManager(net) + == VIR_NETWORK_BRIDGE_MAC_TABLE_MANAGER_LIBVIRT) { + /* libvirt is managing the FDB of the bridge this device + * is attaching to, so we need to turn off learning and + * unicast_flood on the device to prevent the kernel from + * adding any FDB entries for it. We will add an fdb + * entry ourselves (during virDomainInterfaceStartDevices(), + * using the MAC address from the interface config. + */ + if (virNetDevBridgePortSetLearning(brname, net->ifname, false) < 0) + goto cleanup; + if (virNetDevBridgePortSetUnicastFlood(brname, net->ifname, false) < 0) + goto cleanup; + } + } else { + if (virDomainCreateInBridgePortWithHelper(bridgeHelperName, brname, + &net->ifname, + tapfd, + tap_create_flags) < 0) { + virDomainAuditNetDevice(def, net, tunpath, false); + goto cleanup; + } + /* virDomainCreateInBridgePortWithHelper can only create a single FD */ + if (tapfdSize > 1) { + VIR_WARN("Ignoring multiqueue network request"); + tapfdSize = 1; + } + } + + virDomainAuditNetDevice(def, net, tunpath, true); + + if (macFilter && + ebtablesAddForwardAllowIn(ebtables, + net->ifname, + &net->mac) < 0) + goto cleanup; + + if (net->filter && + virDomainConfNWFilterInstantiate(def->name, def->uuid, net, false) < 0) { + goto cleanup; + } + + ret = 0; + + cleanup: + if (ret < 0) { + size_t i; + for (i = 0; i < tapfdSize && tapfd[i] >= 0; i++) + VIR_FORCE_CLOSE(tapfd[i]); + if (template_ifname) + VIR_FREE(net->ifname); + } + + return ret; +} diff --git a/src/hypervisor/domain_interface.h b/src/hypervisor/domain_interface.h index 572b4dd8c5..6d06fe2499 100644 --- a/src/hypervisor/domain_interface.h +++ b/src/hypervisor/domain_interface.h @@ -48,3 +48,13 @@ int virDomainInterfaceClearQoS(virDomainDef *def, virDomainNetDef *net); void virDomainClearNetBandwidth(virDomainDef *def) ATTRIBUTE_NONNULL(1); + +int virDomainInterfaceBridgeConnect(virDomainDef *def, + virDomainNetDef *net, + int *tapfd, + size_t tapfdSize, + bool privileged, + ebtablesContext *ebtables, + bool macFilter, + const char *bridgeHelperName) + ATTRIBUTE_NONNULL(2) G_NO_INLINE; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d15d6a6a9d..8132a17c28 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1646,6 +1646,7 @@ virDomainDriverSetupPersistentDefBlkioParams; # hypervisor/domain_interface.h virDomainClearNetBandwidth; +virDomainInterfaceBridgeConnect; virDomainInterfaceClearQoS; virDomainInterfaceDeleteDevice; virDomainInterfaceEthernetConnect; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f15e6bda1e..0d22bebe89 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8626,6 +8626,7 @@ qemuBuildInterfaceConnect(virDomainObj *vm, bool vhostfd = false; /* also used to signal processing of tapfds */ size_t tapfdSize = net->driver.virtio.queues; g_autofree int *tapfd = g_new0(int, tapfdSize + 1); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); memset(tapfd, -1, (tapfdSize + 1) * sizeof(*tapfd)); @@ -8636,8 +8637,12 @@ qemuBuildInterfaceConnect(virDomainObj *vm, case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_BRIDGE: vhostfd = true; - if (qemuInterfaceBridgeConnect(vm->def, priv->driver, net, - tapfd, &tapfdSize) < 0) + if (virDomainInterfaceBridgeConnect(vm->def, net, + tapfd, tapfdSize, + priv->driver->privileged, + priv->driver->ebtables, + priv->driver->config->macFilter, + cfg->bridgeHelperName) < 0) return -1; break; diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index c2007c7043..23a23d201a 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -34,6 +34,7 @@ #include "virnetdevbridge.h" #include "virnetdevvportprofile.h" #include "virsocket.h" +#include "vircommand.h" #include <sys/stat.h> #include <fcntl.h> @@ -99,224 +100,6 @@ qemuInterfaceDirectConnect(virDomainDef *def, } -/** - * qemuCreateInBridgePortWithHelper: - * @cfg: the configuration object in which the helper name is looked up - * @brname: the bridge name - * @ifname: the returned interface name - * @macaddr: the returned MAC address - * @tapfd: file descriptor return value for the new tap device - * @flags: OR of virNetDevTapCreateFlags: - - * VIR_NETDEV_TAP_CREATE_VNET_HDR - * - Enable IFF_VNET_HDR on the tap device - * - * This function creates a new tap device on a bridge using an external - * helper. The final name for the bridge will be stored in @ifname. - * - * Returns 0 in case of success or -1 on failure - */ -static int -qemuCreateInBridgePortWithHelper(virQEMUDriverConfig *cfg, - const char *brname, - char **ifname, - int *tapfd, - unsigned int flags) -{ - const char *const bridgeHelperDirs[] = { - "/usr/libexec", - "/usr/lib/qemu", - "/usr/lib", - NULL, - }; - g_autoptr(virCommand) cmd = NULL; - g_autofree char *bridgeHelperPath = NULL; - char *errbuf = NULL, *cmdstr = NULL; - int pair[2] = { -1, -1 }; - - if ((flags & ~VIR_NETDEV_TAP_CREATE_VNET_HDR) != VIR_NETDEV_TAP_CREATE_IFUP) - return -1; - - if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) { - virReportSystemError(errno, "%s", _("failed to create socket")); - return -1; - } - - bridgeHelperPath = virFindFileInPathFull(cfg->bridgeHelperName, bridgeHelperDirs); - - if (!bridgeHelperPath) { - virReportSystemError(errno, _("'%1$s' is not a suitable bridge helper"), - cfg->bridgeHelperName); - return -1; - } - - VIR_DEBUG("Using qemu-bridge-helper: %s", bridgeHelperPath); - - cmd = virCommandNew(bridgeHelperPath); - if (flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) - virCommandAddArgFormat(cmd, "--use-vnet"); - virCommandAddArgFormat(cmd, "--br=%s", brname); - virCommandAddArgFormat(cmd, "--fd=%d", pair[1]); - virCommandSetErrorBuffer(cmd, &errbuf); - virCommandDoAsyncIO(cmd); - virCommandPassFD(cmd, pair[1], - VIR_COMMAND_PASS_FD_CLOSE_PARENT); - virCommandClearCaps(cmd); -#ifdef CAP_NET_ADMIN - virCommandAllowCap(cmd, CAP_NET_ADMIN); -#endif - if (virCommandRunAsync(cmd, NULL) < 0) { - *tapfd = -1; - goto cleanup; - } - - do { - *tapfd = virSocketRecvFD(pair[0], 0); - } while (*tapfd < 0 && errno == EINTR); - - if (*tapfd < 0) { - char *errstr = NULL; - - if (!(cmdstr = virCommandToString(cmd, false))) - goto cleanup; - virCommandAbort(cmd); - - if (errbuf && *errbuf) - errstr = g_strdup_printf("stderr=%s", errbuf); - - virReportSystemError(errno, - _("%1$s: failed to communicate with bridge helper: %2$s"), - cmdstr, - NULLSTR_EMPTY(errstr)); - VIR_FREE(errstr); - goto cleanup; - } - - if (virNetDevTapGetName(*tapfd, ifname) < 0 || - virCommandWait(cmd, NULL) < 0) { - VIR_FORCE_CLOSE(*tapfd); - *tapfd = -1; - } - - cleanup: - VIR_FREE(cmdstr); - VIR_FREE(errbuf); - VIR_FORCE_CLOSE(pair[0]); - return *tapfd < 0 ? -1 : 0; -} - -/* qemuInterfaceBridgeConnect: - * @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_NETWORK or - * VIR_DOMAIN_NET_TYPE_BRIDGE (i.e. if the connection is made with a tap - * device connecting to a bridge device) - */ -int -qemuInterfaceBridgeConnect(virDomainDef *def, - virQEMUDriver *driver, - virDomainNetDef *net, - int *tapfd, - size_t *tapfdSize) -{ - const char *brname; - int ret = -1; - unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP; - bool template_ifname = false; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - const char *tunpath = "/dev/net/tun"; - - if (net->backend.tap) { - tunpath = net->backend.tap; - if (!driver->privileged) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cannot use custom tap device in session mode")); - goto cleanup; - } - } - - if (!(brname = virDomainNetGetActualBridgeName(net))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing bridge name")); - goto cleanup; - } - - if (!net->ifname) - template_ifname = true; - - if (virDomainInterfaceIsVnetCompatModel(net)) - tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR; - - if (driver->privileged) { - if (virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, - def->uuid, tunpath, tapfd, *tapfdSize, - virDomainNetGetActualVirtPortProfile(net), - virDomainNetGetActualVlan(net), - virDomainNetGetActualPortOptionsIsolated(net), - net->coalesce, 0, NULL, - tap_create_flags) < 0) { - virDomainAuditNetDevice(def, net, tunpath, false); - goto cleanup; - } - if (virDomainNetGetActualBridgeMACTableManager(net) - == VIR_NETWORK_BRIDGE_MAC_TABLE_MANAGER_LIBVIRT) { - /* libvirt is managing the FDB of the bridge this device - * is attaching to, so we need to turn off learning and - * unicast_flood on the device to prevent the kernel from - * adding any FDB entries for it. We will add an fdb - * entry ourselves (during virDomainInterfaceStartDevices(), - * using the MAC address from the interface config. - */ - if (virNetDevBridgePortSetLearning(brname, net->ifname, false) < 0) - goto cleanup; - if (virNetDevBridgePortSetUnicastFlood(brname, net->ifname, false) < 0) - goto cleanup; - } - } else { - if (qemuCreateInBridgePortWithHelper(cfg, brname, - &net->ifname, - tapfd, tap_create_flags) < 0) { - virDomainAuditNetDevice(def, net, tunpath, false); - goto cleanup; - } - /* qemuCreateInBridgePortWithHelper can only create a single FD */ - if (*tapfdSize > 1) { - VIR_WARN("Ignoring multiqueue network request"); - *tapfdSize = 1; - } - } - - virDomainAuditNetDevice(def, net, tunpath, true); - - if (cfg->macFilter && - ebtablesAddForwardAllowIn(driver->ebtables, - net->ifname, - &net->mac) < 0) - goto cleanup; - - if (net->filter && - virDomainConfNWFilterInstantiate(def->name, def->uuid, net, false) < 0) { - goto cleanup; - } - - ret = 0; - - cleanup: - if (ret < 0) { - size_t i; - for (i = 0; i < *tapfdSize && tapfd[i] >= 0; i++) - VIR_FORCE_CLOSE(tapfd[i]); - if (template_ifname) - VIR_FREE(net->ifname); - } - - return ret; -} - - /* * Returns: -1 on error, 0 on success. Populates net->privateData->slirp if * the slirp helper is needed. -- 2.44.0

On 8/2/24 00:25, Praveen K Paladugu wrote:
From: Praveen K Paladugu <praveenkpaladugu@gmail.com>
From: Praveen K Paladugu <prapal@linux.microsoft.com>
Move methods to connect domain interfaces to host bridges to hypervisor. This is to allow reuse between qemu and ch drivers.
Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/hypervisor/domain_interface.c | 228 ++++++++++++++++++++++++++++++ src/hypervisor/domain_interface.h | 10 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 9 +- src/qemu/qemu_interface.c | 219 +--------------------------- 5 files changed, 247 insertions(+), 220 deletions(-)
diff --git a/src/hypervisor/domain_interface.c b/src/hypervisor/domain_interface.c index 756abb08e9..0c19ff859a 100644 --- a/src/hypervisor/domain_interface.c +++ b/src/hypervisor/domain_interface.c @@ -39,6 +39,7 @@ #include "virnetdevmidonet.h" #include "virnetdevopenvswitch.h" #include "virnetdevtap.h" +#include "vircommand.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -514,3 +515,230 @@ virDomainClearNetBandwidth(virDomainDef *def) virDomainInterfaceClearQoS(def, def->nets[i]); } } + +/** + * virDomainCreateInBridgePortWithHelper: + * @bridgeHelperName: name of the bridge helper program + * @brname: the bridge name + * @ifname: the returned interface name + * @tapfd: file descriptor return value for the new tap device + * @flags: OR of virNetDevTapCreateFlags: + + * VIR_NETDEV_TAP_CREATE_VNET_HDR + * - Enable IFF_VNET_HDR on the tap device + * + * This function creates a new tap device on a bridge using an external + * helper. The final name for the bridge will be stored in @ifname. + * + * Returns 0 in case of success or -1 on failure + */ +static int +virDomainCreateInBridgePortWithHelper(const char *bridgeHelperName, + const char *brname, + char **ifname, + int *tapfd, + unsigned int flags) +{ + const char *const bridgeHelperDirs[] = { + "/usr/libexec", + "/usr/lib/qemu", + "/usr/lib", + NULL, + }; + g_autoptr(virCommand) cmd = NULL; + g_autofree char *bridgeHelperPath = NULL; + char *errbuf = NULL, *cmdstr = NULL; + int pair[2] = { -1, -1 }; + + if (!bridgeHelperName) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing bridge helper name")); + return -1; + } + + if ((flags & ~VIR_NETDEV_TAP_CREATE_VNET_HDR) != VIR_NETDEV_TAP_CREATE_IFUP) + return -1; + + if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) { + virReportSystemError(errno, "%s", _("failed to create socket")); + return -1; + } + + bridgeHelperPath = virFindFileInPathFull(bridgeHelperName, bridgeHelperDirs); + + if (!bridgeHelperPath) { + virReportSystemError(errno, _("'%1$s' is not a suitable bridge helper"), + bridgeHelperName); + return -1; + } + + VIR_DEBUG("Using qemu-bridge-helper: %s", bridgeHelperPath); + cmd = virCommandNew(bridgeHelperPath); + if (flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) + virCommandAddArgFormat(cmd, "--use-vnet"); + virCommandAddArgFormat(cmd, "--br=%s", brname); + virCommandAddArgFormat(cmd, "--fd=%d", pair[1]); + virCommandSetErrorBuffer(cmd, &errbuf); + virCommandDoAsyncIO(cmd); + virCommandPassFD(cmd, pair[1], + VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandClearCaps(cmd); +#ifdef CAP_NET_ADMIN + virCommandAllowCap(cmd, CAP_NET_ADMIN); +#endif + if (virCommandRunAsync(cmd, NULL) < 0) { + *tapfd = -1; + goto cleanup; + } + + do { + *tapfd = virSocketRecvFD(pair[0], 0); + } while (*tapfd < 0 && errno == EINTR); + + if (*tapfd < 0) { + char *errstr = NULL; + + if (!(cmdstr = virCommandToString(cmd, false))) + goto cleanup; + virCommandAbort(cmd); + + if (errbuf && *errbuf) + errstr = g_strdup_printf("stderr=%s", errbuf); + + virReportSystemError(errno, + _("%1$s: failed to communicate with bridge helper: %2$s"), + cmdstr, + NULLSTR_EMPTY(errstr)); + VIR_FREE(errstr); + goto cleanup; + } + + if (virNetDevTapGetName(*tapfd, ifname) < 0 || + virCommandWait(cmd, NULL) < 0) { + VIR_FORCE_CLOSE(*tapfd); + *tapfd = -1; + } + + cleanup: + VIR_FREE(cmdstr); + VIR_FREE(errbuf); + VIR_FORCE_CLOSE(pair[0]); + return *tapfd < 0 ? -1 : 0; +} + + +/* virDomainInterfaceBridgeConnect: + * @def: the definition of the VM + * @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 + * @privileged: whether running as privileged user + * @ebtables: ebtales context + * @macFilter: whether driver support mac Filtering + * @bridgeHelperName:name of the bridge helper program to run in non-privileged mode + * + * Called *only* called if actualType is VIR_DOMAIN_NET_TYPE_NETWORK or + * VIR_DOMAIN_NET_TYPE_BRIDGE (i.e. if the connection is made with a tap + * device connecting to a bridge device) + */ +int +virDomainInterfaceBridgeConnect(virDomainDef *def, + virDomainNetDef *net, + int *tapfd, + size_t tapfdSize,
Since this ^^ is not passed as a pointer, then ...
+ bool privileged, + ebtablesContext *ebtables, + bool macFilter, + const char *bridgeHelperName) +{ + const char *brname; + int ret = -1; + unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP; + bool template_ifname = false; + const char *tunpath = "/dev/net/tun"; + + 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; + } + } + + if (!(brname = virDomainNetGetActualBridgeName(net))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing bridge name")); + goto cleanup; + } + + if (!net->ifname) + template_ifname = true; + + if (virDomainInterfaceIsVnetCompatModel(net)) + tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR; + + if (privileged) { + if (virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, + def->uuid, tunpath, tapfd, tapfdSize, + virDomainNetGetActualVirtPortProfile(net), + virDomainNetGetActualVlan(net), + virDomainNetGetActualPortOptionsIsolated(net), + net->coalesce, 0, NULL, + tap_create_flags) < 0) { + virDomainAuditNetDevice(def, net, tunpath, false); + goto cleanup; + } + if (virDomainNetGetActualBridgeMACTableManager(net) + == VIR_NETWORK_BRIDGE_MAC_TABLE_MANAGER_LIBVIRT) { + /* libvirt is managing the FDB of the bridge this device + * is attaching to, so we need to turn off learning and + * unicast_flood on the device to prevent the kernel from + * adding any FDB entries for it. We will add an fdb + * entry ourselves (during virDomainInterfaceStartDevices(), + * using the MAC address from the interface config. + */ + if (virNetDevBridgePortSetLearning(brname, net->ifname, false) < 0) + goto cleanup; + if (virNetDevBridgePortSetUnicastFlood(brname, net->ifname, false) < 0) + goto cleanup; + } + } else { + if (virDomainCreateInBridgePortWithHelper(bridgeHelperName, brname, + &net->ifname, + tapfd, + tap_create_flags) < 0) { + virDomainAuditNetDevice(def, net, tunpath, false); + goto cleanup; + } + /* virDomainCreateInBridgePortWithHelper can only create a single FD */ + if (tapfdSize > 1) { + VIR_WARN("Ignoring multiqueue network request"); + tapfdSize = 1;
... this has not the desired effect.
+ } + } + + virDomainAuditNetDevice(def, net, tunpath, true); + + if (macFilter && + ebtablesAddForwardAllowIn(ebtables, + net->ifname, + &net->mac) < 0) + goto cleanup; + + if (net->filter && + virDomainConfNWFilterInstantiate(def->name, def->uuid, net, false) < 0) { + goto cleanup; + } + + ret = 0; + + cleanup: + if (ret < 0) { + size_t i; + for (i = 0; i < tapfdSize && tapfd[i] >= 0; i++) + VIR_FORCE_CLOSE(tapfd[i]); + if (template_ifname) + VIR_FREE(net->ifname); + } + + return ret; +} diff --git a/src/hypervisor/domain_interface.h b/src/hypervisor/domain_interface.h index 572b4dd8c5..6d06fe2499 100644 --- a/src/hypervisor/domain_interface.h +++ b/src/hypervisor/domain_interface.h @@ -48,3 +48,13 @@ int virDomainInterfaceClearQoS(virDomainDef *def, virDomainNetDef *net); void virDomainClearNetBandwidth(virDomainDef *def) ATTRIBUTE_NONNULL(1); + +int virDomainInterfaceBridgeConnect(virDomainDef *def, + virDomainNetDef *net, + int *tapfd, + size_t tapfdSize, + bool privileged, + ebtablesContext *ebtables, + bool macFilter, + const char *bridgeHelperName) + ATTRIBUTE_NONNULL(2) G_NO_INLINE; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d15d6a6a9d..8132a17c28 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1646,6 +1646,7 @@ virDomainDriverSetupPersistentDefBlkioParams;
# hypervisor/domain_interface.h virDomainClearNetBandwidth; +virDomainInterfaceBridgeConnect; virDomainInterfaceClearQoS; virDomainInterfaceDeleteDevice; virDomainInterfaceEthernetConnect; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f15e6bda1e..0d22bebe89 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8626,6 +8626,7 @@ qemuBuildInterfaceConnect(virDomainObj *vm, bool vhostfd = false; /* also used to signal processing of tapfds */ size_t tapfdSize = net->driver.virtio.queues; g_autofree int *tapfd = g_new0(int, tapfdSize + 1); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver);
memset(tapfd, -1, (tapfdSize + 1) * sizeof(*tapfd));
@@ -8636,8 +8637,12 @@ qemuBuildInterfaceConnect(virDomainObj *vm, case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_BRIDGE: vhostfd = true; - if (qemuInterfaceBridgeConnect(vm->def, priv->driver, net, - tapfd, &tapfdSize) < 0)
There are two more occurances of qemuInterfaceBridgeConnect(): one in qemu_interface.h (which needs to be removed) and more crucially the other in qemuxml2argvmock.c because otherwise qemuxmlconftest fails.
+ if (virDomainInterfaceBridgeConnect(vm->def, net, + tapfd, tapfdSize, + priv->driver->privileged, + priv->driver->ebtables, + priv->driver->config->macFilter, + cfg->bridgeHelperName) < 0) return -1; break;
Michal

From: Praveen K Paladugu <praveenkpaladugu@gmail.com> From: Praveen K Paladugu <prapal@linux.microsoft.com> enable VIR_DOMAIN_NET_TYPE_NETWORK network support for ch guests. Tested with following config: <interface type='network'> <source network="default" bridge='virbr0'/> <model type='virtio'/> <driver queues="1"/> </interface> Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/ch/ch_interface.c | 57 +++++++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 15 deletions(-) diff --git a/src/ch/ch_interface.c b/src/ch/ch_interface.c index c7af6a35fa..07bdd71560 100644 --- a/src/ch/ch_interface.c +++ b/src/ch/ch_interface.c @@ -28,7 +28,7 @@ #include "ch_interface.h" #include "virjson.h" #include "virlog.h" - +#include "datatypes.h" #define VIR_FROM_THIS VIR_FROM_CH @@ -39,8 +39,9 @@ VIR_LOG_INIT("ch.ch_interface"); * @driver: pointer to ch driver object * @vm: pointer to domain definition * @net: pointer to a guest net - * @nicindexes: returned array of FDs of guest interfaces - * @nnicindexes: returned number of guest interfaces + * @tapfds: returned array of tap FDs + * @nicindexes: returned array list of network interface indexes + * @nnicindexes: returned number of network interfaces * * * Returns 0 on success, -1 on error. @@ -49,10 +50,24 @@ int virCHConnetNetworkInterfaces(virCHDriver *driver, virDomainDef *vm, virDomainNetDef *net, - int *tapfds, int **nicindexes, size_t *nnicindexes) + int *tapfds, + int **nicindexes, size_t *nnicindexes) { virDomainNetType actualType = virDomainNetGetActualType(net); + g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); + g_autoptr(virConnect) conn = NULL; + + /* If appropriate, grab a physical device from the configured + * network's pool of devices, or resolve bridge device name + * to the one defined in the network definition. + */ + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + if (!(conn = virGetConnectNetwork())) + return -1; + if (virDomainNetAllocateActualDevice(conn, vm, net) < 0) + return -1; + } switch (actualType) { case VIR_DOMAIN_NET_TYPE_ETHERNET: @@ -63,20 +78,19 @@ virCHConnetNetworkInterfaces(virCHDriver *driver, net->driver.virtio.queues) < 0) return -1; - G_GNUC_FALLTHROUGH; + break; case VIR_DOMAIN_NET_TYPE_NETWORK: + if (virDomainInterfaceBridgeConnect(vm, net, + tapfds, + net->driver.virtio.queues, + driver->privileged, + driver->ebtables, + false, + NULL) < 0) + return -1; + break; case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_DIRECT: - if (nicindexes && nnicindexes && net->ifname) { - int nicindex = 0; - - if (virNetDevGetIndex(net->ifname, &nicindex) < 0) - return -1; - - VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex); - } - - break; case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: @@ -94,6 +108,19 @@ virCHConnetNetworkInterfaces(virCHDriver *driver, _("Unsupported Network type %1$d"), actualType); return -1; } + if ( actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || + actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_DIRECT ) { + if (nicindexes && nnicindexes && net->ifname) { + int nicindex = 0; + + if (virNetDevGetIndex(net->ifname, &nicindex) < 0) + return -1; + + VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex); + } + } return 0; } -- 2.44.0

On Thu, Aug 01, 2024 at 05:25:14PM -0500, Praveen K Paladugu wrote:
From: Praveen K Paladugu <praveenkpaladugu@gmail.com>
From: Praveen K Paladugu <prapal@linux.microsoft.com>
enable VIR_DOMAIN_NET_TYPE_NETWORK network support for ch guests. Tested with following config:
<interface type='network'> <source network="default" bridge='virbr0'/> <model type='virtio'/> <driver queues="1"/> </interface>
Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/ch/ch_interface.c | 57 +++++++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 15 deletions(-)
diff --git a/src/ch/ch_interface.c b/src/ch/ch_interface.c index c7af6a35fa..07bdd71560 100644 --- a/src/ch/ch_interface.c +++ b/src/ch/ch_interface.c @@ -28,7 +28,7 @@ #include "ch_interface.h" #include "virjson.h" #include "virlog.h" - +#include "datatypes.h"
#define VIR_FROM_THIS VIR_FROM_CH
@@ -39,8 +39,9 @@ VIR_LOG_INIT("ch.ch_interface"); * @driver: pointer to ch driver object * @vm: pointer to domain definition * @net: pointer to a guest net - * @nicindexes: returned array of FDs of guest interfaces - * @nnicindexes: returned number of guest interfaces + * @tapfds: returned array of tap FDs + * @nicindexes: returned array list of network interface indexes + * @nnicindexes: returned number of network interfaces * * * Returns 0 on success, -1 on error. @@ -49,10 +50,24 @@ int virCHConnetNetworkInterfaces(virCHDriver *driver, virDomainDef *vm, virDomainNetDef *net, - int *tapfds, int **nicindexes, size_t *nnicindexes) + int *tapfds, + int **nicindexes, size_t *nnicindexes) { virDomainNetType actualType = virDomainNetGetActualType(net); + g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); + g_autoptr(virConnect) conn = NULL; +
+ /* If appropriate, grab a physical device from the configured + * network's pool of devices, or resolve bridge device name + * to the one defined in the network definition. + */ + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + if (!(conn = virGetConnectNetwork())) + return -1; + if (virDomainNetAllocateActualDevice(conn, vm, net) < 0) + return -1; + }
switch (actualType) { case VIR_DOMAIN_NET_TYPE_ETHERNET: @@ -63,20 +78,19 @@ virCHConnetNetworkInterfaces(virCHDriver *driver, net->driver.virtio.queues) < 0) return -1;
- G_GNUC_FALLTHROUGH; + break; case VIR_DOMAIN_NET_TYPE_NETWORK: + if (virDomainInterfaceBridgeConnect(vm, net, + tapfds, + net->driver.virtio.queues, + driver->privileged, + driver->ebtables, + false, + NULL) < 0) + return -1; + break; case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_DIRECT: - if (nicindexes && nnicindexes && net->ifname) { - int nicindex = 0; - - if (virNetDevGetIndex(net->ifname, &nicindex) < 0) - return -1; - - VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex); - } - - break; case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: @@ -94,6 +108,19 @@ virCHConnetNetworkInterfaces(virCHDriver *driver, _("Unsupported Network type %1$d"), actualType); return -1; } + if ( actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || + actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_DIRECT ) { + if (nicindexes && nnicindexes && net->ifname) { + int nicindex = 0; + + if (virNetDevGetIndex(net->ifname, &nicindex) < 0) + return -1; + + VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex); + } + }
Coverity complains here that this code is unreachable, which is not true but moving it here after the switch makes regression to the original code. With this change it will never be done for VIR_DOMAIN_NET_TYPE_BRIDGE or VIR_DOMAIN_NET_TYPE_DIRECT and both network types will result in error "Unsupported Network type ...". Pavel
return 0; } -- 2.44.0

On 8/27/2024 5:18 AM, Pavel Hrdina wrote:
On Thu, Aug 01, 2024 at 05:25:14PM -0500, Praveen K Paladugu wrote:
From: Praveen K Paladugu <praveenkpaladugu@gmail.com>
From: Praveen K Paladugu <prapal@linux.microsoft.com>
enable VIR_DOMAIN_NET_TYPE_NETWORK network support for ch guests. Tested with following config:
<interface type='network'> <source network="default" bridge='virbr0'/> <model type='virtio'/> <driver queues="1"/> </interface>
Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/ch/ch_interface.c | 57 +++++++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 15 deletions(-)
diff --git a/src/ch/ch_interface.c b/src/ch/ch_interface.c index c7af6a35fa..07bdd71560 100644 --- a/src/ch/ch_interface.c +++ b/src/ch/ch_interface.c @@ -28,7 +28,7 @@ #include "ch_interface.h" #include "virjson.h" #include "virlog.h" - +#include "datatypes.h"
#define VIR_FROM_THIS VIR_FROM_CH
@@ -39,8 +39,9 @@ VIR_LOG_INIT("ch.ch_interface"); * @driver: pointer to ch driver object * @vm: pointer to domain definition * @net: pointer to a guest net - * @nicindexes: returned array of FDs of guest interfaces - * @nnicindexes: returned number of guest interfaces + * @tapfds: returned array of tap FDs + * @nicindexes: returned array list of network interface indexes + * @nnicindexes: returned number of network interfaces * * * Returns 0 on success, -1 on error. @@ -49,10 +50,24 @@ int virCHConnetNetworkInterfaces(virCHDriver *driver, virDomainDef *vm, virDomainNetDef *net, - int *tapfds, int **nicindexes, size_t *nnicindexes) + int *tapfds, + int **nicindexes, size_t *nnicindexes) { virDomainNetType actualType = virDomainNetGetActualType(net); + g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); + g_autoptr(virConnect) conn = NULL; +
+ /* If appropriate, grab a physical device from the configured + * network's pool of devices, or resolve bridge device name + * to the one defined in the network definition. + */ + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + if (!(conn = virGetConnectNetwork())) + return -1; + if (virDomainNetAllocateActualDevice(conn, vm, net) < 0) + return -1; + }
switch (actualType) { case VIR_DOMAIN_NET_TYPE_ETHERNET: @@ -63,20 +78,19 @@ virCHConnetNetworkInterfaces(virCHDriver *driver, net->driver.virtio.queues) < 0) return -1;
- G_GNUC_FALLTHROUGH; + break; case VIR_DOMAIN_NET_TYPE_NETWORK: + if (virDomainInterfaceBridgeConnect(vm, net, + tapfds, + net->driver.virtio.queues, + driver->privileged, + driver->ebtables, + false, + NULL) < 0) + return -1; + break; case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_DIRECT: - if (nicindexes && nnicindexes && net->ifname) { - int nicindex = 0; - - if (virNetDevGetIndex(net->ifname, &nicindex) < 0) - return -1; - - VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex); - } - - break; case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: @@ -94,6 +108,19 @@ virCHConnetNetworkInterfaces(virCHDriver *driver, _("Unsupported Network type %1$d"), actualType); return -1; } + if ( actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || + actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_DIRECT ) { + if (nicindexes && nnicindexes && net->ifname) { + int nicindex = 0; + + if (virNetDevGetIndex(net->ifname, &nicindex) < 0) + return -1; + + VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex); + } + }
Coverity complains here that this code is unreachable, which is not true but moving it here after the switch makes regression to the original code. With this change it will never be done for VIR_DOMAIN_NET_TYPE_BRIDGE or VIR_DOMAIN_NET_TYPE_DIRECT and both network types will result in error "Unsupported Network type ...".
Pavel
I don't follow your claim about "makes regression to the original code". Here "actualType" is evaluated at the top and checked after the switch case. If "actualType" if any of the above types, nicindexes will be updated appropriately. If the concern is about connecting the interface to configured bridge, that is handled elsewhere. For example: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/M6ZNU... This enables Bridge mode networking.
return 0; } -- 2.44.0
-- Regards, Praveen K Paladugu

On Tue, Aug 27, 2024 at 11:05:39AM -0500, Praveen K Paladugu wrote:
On 8/27/2024 5:18 AM, Pavel Hrdina wrote:
On Thu, Aug 01, 2024 at 05:25:14PM -0500, Praveen K Paladugu wrote:
From: Praveen K Paladugu <praveenkpaladugu@gmail.com>
From: Praveen K Paladugu <prapal@linux.microsoft.com>
enable VIR_DOMAIN_NET_TYPE_NETWORK network support for ch guests. Tested with following config:
<interface type='network'> <source network="default" bridge='virbr0'/> <model type='virtio'/> <driver queues="1"/> </interface>
Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/ch/ch_interface.c | 57 +++++++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 15 deletions(-)
diff --git a/src/ch/ch_interface.c b/src/ch/ch_interface.c index c7af6a35fa..07bdd71560 100644 --- a/src/ch/ch_interface.c +++ b/src/ch/ch_interface.c @@ -28,7 +28,7 @@ #include "ch_interface.h" #include "virjson.h" #include "virlog.h" - +#include "datatypes.h" #define VIR_FROM_THIS VIR_FROM_CH @@ -39,8 +39,9 @@ VIR_LOG_INIT("ch.ch_interface"); * @driver: pointer to ch driver object * @vm: pointer to domain definition * @net: pointer to a guest net - * @nicindexes: returned array of FDs of guest interfaces - * @nnicindexes: returned number of guest interfaces + * @tapfds: returned array of tap FDs + * @nicindexes: returned array list of network interface indexes + * @nnicindexes: returned number of network interfaces * * * Returns 0 on success, -1 on error. @@ -49,10 +50,24 @@ int virCHConnetNetworkInterfaces(virCHDriver *driver, virDomainDef *vm, virDomainNetDef *net, - int *tapfds, int **nicindexes, size_t *nnicindexes) + int *tapfds, + int **nicindexes, size_t *nnicindexes) { virDomainNetType actualType = virDomainNetGetActualType(net); + g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); + g_autoptr(virConnect) conn = NULL; + + /* If appropriate, grab a physical device from the configured + * network's pool of devices, or resolve bridge device name + * to the one defined in the network definition. + */ + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + if (!(conn = virGetConnectNetwork())) + return -1; + if (virDomainNetAllocateActualDevice(conn, vm, net) < 0) + return -1; + } switch (actualType) { case VIR_DOMAIN_NET_TYPE_ETHERNET: @@ -63,20 +78,19 @@ virCHConnetNetworkInterfaces(virCHDriver *driver, net->driver.virtio.queues) < 0) return -1; - G_GNUC_FALLTHROUGH; + break; case VIR_DOMAIN_NET_TYPE_NETWORK: + if (virDomainInterfaceBridgeConnect(vm, net, + tapfds, + net->driver.virtio.queues, + driver->privileged, + driver->ebtables, + false, + NULL) < 0) + return -1; + break; case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_DIRECT: - if (nicindexes && nnicindexes && net->ifname) { - int nicindex = 0; - - if (virNetDevGetIndex(net->ifname, &nicindex) < 0) - return -1; - - VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex); - } - - break; case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: @@ -94,6 +108,19 @@ virCHConnetNetworkInterfaces(virCHDriver *driver, _("Unsupported Network type %1$d"), actualType); return -1; } + if ( actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || + actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_DIRECT ) { + if (nicindexes && nnicindexes && net->ifname) { + int nicindex = 0; + + if (virNetDevGetIndex(net->ifname, &nicindex) < 0) + return -1; + + VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex); + } + }
Coverity complains here that this code is unreachable, which is not true but moving it here after the switch makes regression to the original code. With this change it will never be done for VIR_DOMAIN_NET_TYPE_BRIDGE or VIR_DOMAIN_NET_TYPE_DIRECT and both network types will result in error "Unsupported Network type ...".
Pavel
I don't follow your claim about "makes regression to the original code". Here "actualType" is evaluated at the top and checked after the switch case.
If "actualType" if any of the above types, nicindexes will be updated appropriately.
If the concern is about connecting the interface to configured bridge, that is handled elsewhere. For example:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/M6ZNU...
This enables Bridge mode networking.
Let me paste the whole function here as the diff is not complete and hides the issue:
virCHConnetNetworkInterfaces(virCHDriver *driver, virDomainDef *vm, virDomainNetDef *net, int *tapfds, int **nicindexes, size_t *nnicindexes) { virDomainNetType actualType = virDomainNetGetActualType(net); g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); g_autoptr(virConnect) conn = NULL; size_t tapfdSize = net->driver.virtio.queues;
/* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. */ if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (!(conn = virGetConnectNetwork())) return -1; if (virDomainNetAllocateActualDevice(conn, vm, net) < 0) return -1; }
switch (actualType) {
Here we have switch that covers all possible values for actualType ...
case VIR_DOMAIN_NET_TYPE_ETHERNET: if (virDomainInterfaceEthernetConnect(vm, net, driver->ebtables, false, driver->privileged, tapfds, net->driver.virtio.queues) < 0) return -1;
break; case VIR_DOMAIN_NET_TYPE_NETWORK: if (virDomainInterfaceBridgeConnect(vm, net, tapfds, &tapfdSize, driver->privileged, driver->ebtables, false, NULL) < 0) return -1; break; case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_DIRECT:
... here you have cases for bridge and direct types ...
case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: case VIR_DOMAIN_NET_TYPE_MCAST: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: case VIR_DOMAIN_NET_TYPE_INTERNAL: case VIR_DOMAIN_NET_TYPE_HOSTDEV: case VIR_DOMAIN_NET_TYPE_UDP: case VIR_DOMAIN_NET_TYPE_VDPA: case VIR_DOMAIN_NET_TYPE_NULL: case VIR_DOMAIN_NET_TYPE_VDS: case VIR_DOMAIN_NET_TYPE_LAST: default:
... and here in case of bridge or direct type it will result in error and the code that updates nicindexes is never executed.
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported Network type %1$d"), actualType); return -1; }
If the actualType == VIR_DOMAIN_NET_TYPE_BRIDGE we will never get to this part as it will reach the error above and returns from this function, the same happens for VIR_DOMAIN_NET_TYPE_DIRECT. Before this patch actualType VIR_DOMAIN_NET_TYPE_BRIDGE and VIR_DOMAIN_NET_TYPE_DIRECT had nicindexes updated. After this patch only VIR_DOMAIN_NET_TYPE_ETHERNET and VIR_DOMAIN_NET_TYPE_NETWORK have nicindexes updated.
if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { if (nicindexes && nnicindexes && net->ifname) { int nicindex = 0;
if (virNetDevGetIndex(net->ifname, &nicindex) < 0) return -1;
VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex); } }
return 0; }
Pavel
return 0; } -- 2.44.0
-- Regards, Praveen K Paladugu

On 8/27/2024 12:40 PM, Pavel Hrdina wrote:
On Tue, Aug 27, 2024 at 11:05:39AM -0500, Praveen K Paladugu wrote:
On 8/27/2024 5:18 AM, Pavel Hrdina wrote:
On Thu, Aug 01, 2024 at 05:25:14PM -0500, Praveen K Paladugu wrote:
From: Praveen K Paladugu <praveenkpaladugu@gmail.com>
From: Praveen K Paladugu <prapal@linux.microsoft.com>
enable VIR_DOMAIN_NET_TYPE_NETWORK network support for ch guests. Tested with following config:
<interface type='network'> <source network="default" bridge='virbr0'/> <model type='virtio'/> <driver queues="1"/> </interface>
Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/ch/ch_interface.c | 57 +++++++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 15 deletions(-)
diff --git a/src/ch/ch_interface.c b/src/ch/ch_interface.c index c7af6a35fa..07bdd71560 100644 --- a/src/ch/ch_interface.c +++ b/src/ch/ch_interface.c @@ -28,7 +28,7 @@ #include "ch_interface.h" #include "virjson.h" #include "virlog.h" - +#include "datatypes.h" #define VIR_FROM_THIS VIR_FROM_CH @@ -39,8 +39,9 @@ VIR_LOG_INIT("ch.ch_interface"); * @driver: pointer to ch driver object * @vm: pointer to domain definition * @net: pointer to a guest net - * @nicindexes: returned array of FDs of guest interfaces - * @nnicindexes: returned number of guest interfaces + * @tapfds: returned array of tap FDs + * @nicindexes: returned array list of network interface indexes + * @nnicindexes: returned number of network interfaces * * * Returns 0 on success, -1 on error. @@ -49,10 +50,24 @@ int virCHConnetNetworkInterfaces(virCHDriver *driver, virDomainDef *vm, virDomainNetDef *net, - int *tapfds, int **nicindexes, size_t *nnicindexes) + int *tapfds, + int **nicindexes, size_t *nnicindexes) { virDomainNetType actualType = virDomainNetGetActualType(net); + g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); + g_autoptr(virConnect) conn = NULL; + + /* If appropriate, grab a physical device from the configured + * network's pool of devices, or resolve bridge device name + * to the one defined in the network definition. + */ + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + if (!(conn = virGetConnectNetwork())) + return -1; + if (virDomainNetAllocateActualDevice(conn, vm, net) < 0) + return -1; + } switch (actualType) { case VIR_DOMAIN_NET_TYPE_ETHERNET: @@ -63,20 +78,19 @@ virCHConnetNetworkInterfaces(virCHDriver *driver, net->driver.virtio.queues) < 0) return -1; - G_GNUC_FALLTHROUGH; + break; case VIR_DOMAIN_NET_TYPE_NETWORK: + if (virDomainInterfaceBridgeConnect(vm, net, + tapfds, + net->driver.virtio.queues, + driver->privileged, + driver->ebtables, + false, + NULL) < 0) + return -1; + break; case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_DIRECT: - if (nicindexes && nnicindexes && net->ifname) { - int nicindex = 0; - - if (virNetDevGetIndex(net->ifname, &nicindex) < 0) - return -1; - - VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex); - } - - break; case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: @@ -94,6 +108,19 @@ virCHConnetNetworkInterfaces(virCHDriver *driver, _("Unsupported Network type %1$d"), actualType); return -1; } + if ( actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || + actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_DIRECT ) { + if (nicindexes && nnicindexes && net->ifname) { + int nicindex = 0; + + if (virNetDevGetIndex(net->ifname, &nicindex) < 0) + return -1; + + VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex); + } + }
Coverity complains here that this code is unreachable, which is not true but moving it here after the switch makes regression to the original code. With this change it will never be done for VIR_DOMAIN_NET_TYPE_BRIDGE or VIR_DOMAIN_NET_TYPE_DIRECT and both network types will result in error "Unsupported Network type ...".
Pavel
I don't follow your claim about "makes regression to the original code". Here "actualType" is evaluated at the top and checked after the switch case.
If "actualType" if any of the above types, nicindexes will be updated appropriately.
If the concern is about connecting the interface to configured bridge, that is handled elsewhere. For example:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/M6ZNU...
This enables Bridge mode networking.
Let me paste the whole function here as the diff is not complete and hides the issue:
virCHConnetNetworkInterfaces(virCHDriver *driver, virDomainDef *vm, virDomainNetDef *net, int *tapfds, int **nicindexes, size_t *nnicindexes) { virDomainNetType actualType = virDomainNetGetActualType(net); g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); g_autoptr(virConnect) conn = NULL; size_t tapfdSize = net->driver.virtio.queues;
/* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. */ if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (!(conn = virGetConnectNetwork())) return -1; if (virDomainNetAllocateActualDevice(conn, vm, net) < 0) return -1; }
switch (actualType) {
Here we have switch that covers all possible values for actualType ...
case VIR_DOMAIN_NET_TYPE_ETHERNET: if (virDomainInterfaceEthernetConnect(vm, net, driver->ebtables, false, driver->privileged, tapfds, net->driver.virtio.queues) < 0) return -1;
break; case VIR_DOMAIN_NET_TYPE_NETWORK: if (virDomainInterfaceBridgeConnect(vm, net, tapfds, &tapfdSize, driver->privileged, driver->ebtables, false, NULL) < 0) return -1; break; case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_DIRECT:
... here you have cases for bridge and direct types ...
case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: case VIR_DOMAIN_NET_TYPE_MCAST: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: case VIR_DOMAIN_NET_TYPE_INTERNAL: case VIR_DOMAIN_NET_TYPE_HOSTDEV: case VIR_DOMAIN_NET_TYPE_UDP: case VIR_DOMAIN_NET_TYPE_VDPA: case VIR_DOMAIN_NET_TYPE_NULL: case VIR_DOMAIN_NET_TYPE_VDS: case VIR_DOMAIN_NET_TYPE_LAST: default:
... and here in case of bridge or direct type it will result in error and the code that updates nicindexes is never executed.
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported Network type %1$d"), actualType); return -1; }
If the actualType == VIR_DOMAIN_NET_TYPE_BRIDGE we will never get to this part as it will reach the error above and returns from this function, the same happens for VIR_DOMAIN_NET_TYPE_DIRECT.
Before this patch actualType VIR_DOMAIN_NET_TYPE_BRIDGE and VIR_DOMAIN_NET_TYPE_DIRECT had nicindexes updated.
After this patch only VIR_DOMAIN_NET_TYPE_ETHERNET and VIR_DOMAIN_NET_TYPE_NETWORK have nicindexes updated. I see what you are saying. This is intentional. I am enabling these modes one by one. My plan is to move BRIDGE and DIRECT modes to the right place once they are tested end-to-end.
For example https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/M6ZNU..., which enables Bridge mode.
if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { if (nicindexes && nnicindexes && net->ifname) { int nicindex = 0;
if (virNetDevGetIndex(net->ifname, &nicindex) < 0) return -1;
VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex); } }
return 0; }
Pavel
return 0; } -- 2.44.0
-- Regards, Praveen K Paladugu
-- Regards, Praveen K Paladugu

Bubbling up this thread for reviews. On 8/1/2024 5:25 PM, Praveen K Paladugu wrote:
Move additional domain interface management methods to hypervisor and enable NAT mode network support for ch guests.
Praveen K Paladugu (2): hypervisor: Move domain interface mgmt methods ch: Enable NAT Network mode support
src/ch/ch_interface.c | 57 ++++++-- src/hypervisor/domain_interface.c | 228 ++++++++++++++++++++++++++++++ src/hypervisor/domain_interface.h | 10 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 9 +- src/qemu/qemu_interface.c | 219 +--------------------------- 6 files changed, 289 insertions(+), 235 deletions(-)
-- Regards, Praveen K Paladugu

On 8/2/24 00:25, Praveen K Paladugu wrote:
Move additional domain interface management methods to hypervisor and enable NAT mode network support for ch guests.
Praveen K Paladugu (2): hypervisor: Move domain interface mgmt methods ch: Enable NAT Network mode support
src/ch/ch_interface.c | 57 ++++++-- src/hypervisor/domain_interface.c | 228 ++++++++++++++++++++++++++++++ src/hypervisor/domain_interface.h | 10 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 9 +- src/qemu/qemu_interface.c | 219 +--------------------------- 6 files changed, 289 insertions(+), 235 deletions(-)
I'm fixing all the small nits I've found and merging. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal

Thanks Michal!! On 8/26/2024 9:15 AM, Michal Prívozník wrote:
On 8/2/24 00:25, Praveen K Paladugu wrote:
Move additional domain interface management methods to hypervisor and enable NAT mode network support for ch guests.
Praveen K Paladugu (2): hypervisor: Move domain interface mgmt methods ch: Enable NAT Network mode support
src/ch/ch_interface.c | 57 ++++++-- src/hypervisor/domain_interface.c | 228 ++++++++++++++++++++++++++++++ src/hypervisor/domain_interface.h | 10 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 9 +- src/qemu/qemu_interface.c | 219 +--------------------------- 6 files changed, 289 insertions(+), 235 deletions(-)
I'm fixing all the small nits I've found and merging.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Michal
-- Regards, Praveen K Paladugu
participants (3)
-
Michal Prívozník
-
Pavel Hrdina
-
Praveen K Paladugu