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.0On 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/M6ZNU2E3KN6T4ELDNUWT7V65JBP4I3PW/
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/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
>
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/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.
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/M6ZNU2E3KN6T4ELDNUWT7V65JBP4I3PW/,
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
© 2016 - 2024 Red Hat, Inc.