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@xxxxxxxxx>
> > >
> > > From: Praveen K Paladugu <prapal@xxxxxxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > Signed-off-by: Praveen K Paladugu <prapal@xxxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxx/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
>
Attachment:
signature.asc
Description: PGP signature
