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 <[email protected]> > > > > > > From: Praveen K Paladugu <[email protected]> > > > > > > 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 <[email protected]> > > > Signed-off-by: Praveen K Paladugu <[email protected]> > > > --- > > > 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/[email protected]/thread/M6ZNU2E3KN6T4ELDNUWT7V65JBP4I3PW/ > > 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
>
signature.asc
Description: PGP signature
