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

[Praveen K Paladugu <praveenkpaladugu@xxxxxxxxx>]

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.

> >   
> >       return 0;
> >   }
> > --
> > 2.44.0

--
Regards,
Praveen K Paladugu