Re: [PATCH 2/2] ch: Enable NAT Network mode support

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(a)gmail.com&gt; >>> >>> From: Praveen K Paladugu <prapal(a)linux.microsoft.com&gt; >>> >>> 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(a)gmail.com&gt; >>> Signed-off-by: Praveen K Paladugu <prapal(a)linux.microsoft.com&gt; >>> --- >>> 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/M6... > > 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 >