On 1/16/24 22:25, Praveen K Paladugu wrote:
> enable VIR_DOMAIN_NET_TYPE_ETHERNET network support for ch guests.
>
> Tested with following interface config:
>
> <interface type='ethernet'>
> <target dev='chtap0' managed="yes"/>
> <model type='virtio'/>
> <driver queues='2'/>
> <interface>
>
> Signed-off-by: Praveen K Paladugu <prapal@xxxxxxxxxxxxxxxxxxx>
> ---
> po/POTFILES | 1 +
> src/ch/ch_conf.h | 4 +
> src/ch/ch_domain.c | 41 ++++++++
> src/ch/ch_domain.h | 3 +
> src/ch/ch_interface.c | 100 ++++++++++++++++++++
> src/ch/ch_interface.h | 35 +++++++
> src/ch/ch_monitor.c | 213 ++++++++++++++++--------------------------
> src/ch/ch_monitor.h | 7 +-
> src/ch/ch_process.c | 166 +++++++++++++++++++++++++++++++-
> src/ch/meson.build | 2 +
> 10 files changed, 430 insertions(+), 142 deletions(-)
> create mode 100644 src/ch/ch_interface.c
> create mode 100644 src/ch/ch_interface.h
>
> diff --git a/po/POTFILES b/po/POTFILES
> index b594a8dd39..e48b9023e2 100644
> --- a/po/POTFILES
> +++ b/po/POTFILES
> @@ -21,6 +21,7 @@ src/bhyve/bhyve_process.c
> src/ch/ch_conf.c
> src/ch/ch_domain.c
> src/ch/ch_driver.c
> +src/ch/ch_interface.c
> src/ch/ch_monitor.c
> src/ch/ch_process.c
> src/conf/backup_conf.c
> diff --git a/src/ch/ch_conf.h b/src/ch/ch_conf.h
> index 5b9b42540d..579eca894e 100644
> --- a/src/ch/ch_conf.h
> +++ b/src/ch/ch_conf.h
> @@ -23,6 +23,7 @@
> #include "virdomainobjlist.h"
> #include "virthread.h"
> #include "ch_capabilities.h"
> +#include "virebtables.h"
>
> #define CH_DRIVER_NAME "CH"
> #define CH_CMD "cloud-hypervisor"
> @@ -75,6 +76,9 @@ struct _virCHDriver
>
> /* pid file FD, ensures two copies of the driver can't use the same root */
> int lockFD;
> +
> + /* Immutable pointer, lockless APIs. Pointless abstraction */
> + ebtablesContext *ebtables;
> };
>
> virCaps *virCHDriverCapsInit(void);
> diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
> index c0d6c75b1d..25610a9459 100644
> --- a/src/ch/ch_domain.c
> +++ b/src/ch/ch_domain.c
> @@ -22,6 +22,7 @@
>
> #include "ch_domain.h"
> #include "domain_driver.h"
> +#include "domain_validate.h"
> #include "virchrdev.h"
> #include "virlog.h"
> #include "virtime.h"
> @@ -355,3 +356,43 @@ virCHDomainObjFromDomain(virDomainPtr domain)
>
> return vm;
> }
> +
> +int
> +virCHDomainValidateActualNetDef(virDomainNetDef *net)
> +{
> + virDomainNetType actualType = virDomainNetGetActualType(net);
> +
> + /* hypervisor-agnostic validation */
> + if (virDomainActualNetDefValidate(net) < 0)
> + return -1;
> +
> + /* CH specific validation */
> + switch (actualType) {
> + case VIR_DOMAIN_NET_TYPE_ETHERNET:
> + if (net->guestIP.nips > 1) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("ethernet type supports a single guest ip"));
> + return -1;
> + }
> + break;
> + case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> + case VIR_DOMAIN_NET_TYPE_BRIDGE:
> + case VIR_DOMAIN_NET_TYPE_NETWORK:
> + case VIR_DOMAIN_NET_TYPE_DIRECT:
> + 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_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:
> + break;
> + }
> +
> + return 0;
> +}
> diff --git a/src/ch/ch_domain.h b/src/ch/ch_domain.h
> index 4990914e9f..8dea2b2123 100644
> --- a/src/ch/ch_domain.h
> +++ b/src/ch/ch_domain.h
> @@ -75,3 +75,6 @@ virCHDomainGetMachineName(virDomainObj *vm);
>
> virDomainObj *
> virCHDomainObjFromDomain(virDomainPtr domain);
> +
> +int
> +virCHDomainValidateActualNetDef(virDomainNetDef *net);
> diff --git a/src/ch/ch_interface.c b/src/ch/ch_interface.c
> new file mode 100644
> index 0000000000..ba90103c4e
> --- /dev/null
> +++ b/src/ch/ch_interface.c
> @@ -0,0 +1,100 @@
> +
> +/*
> + * Copyright Microsoft Corp. 2023
> + *
> + * ch_interface.c: methods to connect guest interfaces to appropriate host
> + * backends
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <config.h>
> +
> +#include "domain_conf.h"
> +#include "domain_interface.h"
> +#include "virebtables.h"
> +#include "viralloc.h"
> +#include "ch_interface.h"
> +#include "virjson.h"
> +#include "virlog.h"
> +
> +
> +#define VIR_FROM_THIS VIR_FROM_CH
> +
> +VIR_LOG_INIT("ch.ch_interface");
> +
> +/**
> + * virCHConnetNetworkInterfaces:
> + * @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
> + *
> + *
> + * Returns 0 on success, -1 on error.
> + */
> +int
> +virCHConnetNetworkInterfaces(virCHDriver *driver,
> + virDomainDef *vm,
> + virDomainNetDef *net,
> + int *tapfds, int **nicindexes, size_t *nnicindexes)
> +{
> + virDomainNetType actualType = virDomainNetGetActualType(net);
> +
> +
> + switch (actualType) {
> + case VIR_DOMAIN_NET_TYPE_ETHERNET:
> +
> + if (virDomainInterfaceEthernetConnect(vm, net,
> + driver->ebtables, false,
> + driver->privileged, tapfds,
> + net->driver.virtio.queues) < 0)
> + return -1;
> +
> + G_GNUC_FALLTHROUGH;
> + case VIR_DOMAIN_NET_TYPE_NETWORK:
> + 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:
> + 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:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Unsupported Network type %1$d"), actualType);
> + return -1;
> + }
> +
> + return 0;
> +}
> diff --git a/src/ch/ch_interface.h b/src/ch/ch_interface.h
> new file mode 100644
> index 0000000000..df87d4511f
> --- /dev/null
> +++ b/src/ch/ch_interface.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright Microsoft Corp. 2023
> + *
> + * ch_interface.c: methods to connect guest interfaces to appropriate host
> + * backends
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#pragma once
> +
> +
> +#include "ch_conf.h"
> +#include "virconftypes.h"
> +
> +
> +int
> +virCHConnetNetworkInterfaces(virCHDriver *driver,
> + virDomainDef *vmdef,
> + virDomainNetDef *netdef,
> + int *tapfds,
> + int **nicindexes,
> + size_t *nnicindexes);
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index 6f960c3a51..12f2468cc7 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -24,7 +24,11 @@
> #include <unistd.h>
> #include <curl/curl.h>
>
> +#include "datatypes.h"
> +#include "ch_conf.h"
> +#include "ch_interface.h"
> #include "ch_monitor.h"
> +#include "domain_interface.h"
> #include "viralloc.h"
> #include "vircommand.h"
> #include "virerror.h"
> @@ -258,148 +262,98 @@ virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef)
> return 0;
> }
>
> -static int
> -virCHMonitorBuildNetJson(virJSONValue *nets,
> - virDomainNetDef *netdef,
> - size_t *nnicindexes,
> - int **nicindexes)
> +/**
> + * virCHMonitorBuildNetJson:
> + * @net: pointer to a guest network definition
> + * @jsonstr: returned network json
> + *
> + * Build net json to send to CH
> + * Returns 0 on success or -1 in case of error
> + */
> +int
> +virCHMonitorBuildNetJson(virDomainNetDef *net, char **jsonstr)
> {
> - virDomainNetType netType = virDomainNetGetActualType(netdef);
> char macaddr[VIR_MAC_STRING_BUFLEN];
> - g_autoptr(virJSONValue) net = NULL;
> -
> - // check net type at first
> - net = virJSONValueNewObject();
> -
> - switch (netType) {
> - case VIR_DOMAIN_NET_TYPE_ETHERNET:
> - if (netdef->guestIP.nips == 1) {
> - const virNetDevIPAddr *ip = netdef->guestIP.ips[0];
> - g_autofree char *addr = NULL;
> - virSocketAddr netmask;
> - g_autofree char *netmaskStr = NULL;
> -
> - if (!(addr = virSocketAddrFormat(&ip->address)))
> - return -1;
> - if (virJSONValueObjectAppendString(net, "ip", addr) < 0)
> - return -1;
> -
> - if (virSocketAddrPrefixToNetmask(ip->prefix, &netmask, AF_INET) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to translate net prefix %1$d to netmask"),
> - ip->prefix);
> - return -1;
> - }
> - if (!(netmaskStr = virSocketAddrFormat(&netmask)))
> - return -1;
> - if (virJSONValueObjectAppendString(net, "mask", netmaskStr) < 0)
> - return -1;
> - } else if (netdef->guestIP.nips > 1) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("ethernet type supports a single guest ip"));
> - }
> + g_autoptr(virJSONValue) net_json = virJSONValueNewObject();
> + virDomainNetType actualType = virDomainNetGetActualType(net);
>
> - /* network and bridge use a tap device, and direct uses a
> - * macvtap device
> - */
> - if (nicindexes && nnicindexes && netdef->ifname) {
> - int nicindex = 0;
>
> - if (virNetDevGetIndex(netdef->ifname, &nicindex) < 0)
> - return -1;
>
> - VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex);
> - }
> - break;
> - case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> - if ((virDomainChrType)netdef->data.vhostuser->type != VIR_DOMAIN_CHR_TYPE_UNIX) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("vhost_user type support UNIX socket in this CH"));
> - return -1;
> - } else {
> - if (virJSONValueObjectAppendString(net, "vhost_socket", netdef->data.vhostuser->data.nix.path) < 0)
> - return -1;
> - if (virJSONValueObjectAppendBoolean(net, "vhost_user", true) < 0)
> - return -1;
> - }
> - break;
> - case VIR_DOMAIN_NET_TYPE_BRIDGE:
> - case VIR_DOMAIN_NET_TYPE_NETWORK:
> - case VIR_DOMAIN_NET_TYPE_DIRECT:
> - 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_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:
> - virReportEnumRangeError(virDomainNetType, netType);
> - return -1;
> - }
> -
> - if (netdef->ifname != NULL) {
> - if (virJSONValueObjectAppendString(net, "tap", netdef->ifname) < 0)
> - return -1;
> - }
> - if (virJSONValueObjectAppendString(net, "mac", virMacAddrFormat(&netdef->mac, macaddr)) < 0)
> - return -1;
> + if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
> + const virNetDevIPAddr *ip;
> + g_autofree char *addr = NULL;
> + virSocketAddr netmask;
> + g_autofree char *netmaskStr = NULL;
> +
> + /* nips should be 1, this is already validated in
> + virCHDomainValidateActualNetDef */
> + if (net->guestIP.nips != 1)
> + return -1;
>
> + ip = net->guestIP.ips[0];
>
> - if (netdef->virtio != NULL) {
> - if (netdef->virtio->iommu == VIR_TRISTATE_SWITCH_ON) {
> - if (virJSONValueObjectAppendBoolean(net, "iommu", true) < 0)
> - return -1;
> - }
> - }
> - if (netdef->driver.virtio.queues) {
> - if (virJSONValueObjectAppendNumberInt(net, "num_queues", netdef->driver.virtio.queues) < 0)
> +
> + if (!(addr = virSocketAddrFormat(&ip->address)))
> return -1;
> - }
>
> - if (netdef->driver.virtio.rx_queue_size || netdef->driver.virtio.tx_queue_size) {
> - if (netdef->driver.virtio.rx_queue_size != netdef->driver.virtio.tx_queue_size) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("virtio rx_queue_size option %1$d is not same with tx_queue_size %2$d"),
> - netdef->driver.virtio.rx_queue_size,
> - netdef->driver.virtio.tx_queue_size);
> + if (virJSONValueObjectAppendString(net_json, "ip", addr) < 0)
> + return -1;
> +
> + if (virSocketAddrPrefixToNetmask(ip->prefix, &netmask, AF_INET) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to translate net prefix %1$d to netmask"),
> + ip->prefix);
> return -1;
> }
> - if (virJSONValueObjectAppendNumberInt(net, "queue_size", netdef->driver.virtio.rx_queue_size) < 0)
> +
> + if (!(netmaskStr = virSocketAddrFormat(&netmask)))
> + return -1;
> + if (virJSONValueObjectAppendString(net_json, "mask", netmaskStr) < 0)
> return -1;
> }
>
> - if (virJSONValueArrayAppend(nets, &net) < 0)
> + if (virJSONValueObjectAppendString(net_json, "mac",
> + virMacAddrFormat(&net->mac, macaddr)) < 0)
> return -1;
>
> - return 0;
> -}
> -
> -static int
> -virCHMonitorBuildNetsJson(virJSONValue *content,
> - virDomainDef *vmdef,
> - size_t *nnicindexes,
> - int **nicindexes)
> -{
> - g_autoptr(virJSONValue) nets = NULL;
> - size_t i;
> + if (net->virtio != NULL) {
> + if (net->virtio->iommu == VIR_TRISTATE_SWITCH_ON) {
> + if (virJSONValueObjectAppendBoolean(net_json, "iommu", true) < 0)
> + return -1;
> + }
> + }
>
> - if (vmdef->nnets > 0) {
> - nets = virJSONValueNewArray();
> + /* Cloud-Hypervisor expects number of queues. 1 for rx and 1 for tx.
> + * Multiply queue pairs by 2 to provide total number of queues to CH
> + */
> + if (net->driver.virtio.queues) {
> + if (virJSONValueObjectAppendNumberInt(net_json, "num_queues",
> + 2 * net->driver.virtio.queues) < 0)
> + return -1;
> + }
>
> - for (i = 0; i < vmdef->nnets; i++) {
> - if (virCHMonitorBuildNetJson(nets, vmdef->nets[i],
> - nnicindexes, nicindexes) < 0)
> - return -1;
> + if (net->driver.virtio.rx_queue_size || net->driver.virtio.tx_queue_size) {
> + if (net->driver.virtio.rx_queue_size !=
> + net->driver.virtio.tx_queue_size) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("virtio rx_queue_size option %1$d is not same with tx_queue_size %2$d"),
> + net->driver.virtio.rx_queue_size,
> + net->driver.virtio.tx_queue_size);
> + return -1;
> }
> - if (virJSONValueObjectAppend(content, "net", &nets) < 0)
> + if (virJSONValueObjectAppendNumberInt(net_json, "queue_size",
> + net->driver.virtio.rx_queue_size) < 0)
> + return -1;
> + }
> +
> + if (net->mtu) {
> + if (virJSONValueObjectAppendNumberInt(net_json, "mtu", net->mtu) < 0)
> return -1;
> }
>
> + if (!(*jsonstr = virJSONValueToString(net_json, false)))
> + return -1;
> +
> return 0;
> }
>
> @@ -456,11 +410,8 @@ virCHMonitorBuildDevicesJson(virJSONValue *content,
> }
>
> static int
> -virCHMonitorBuildVMJson(virCHDriver *driver,
> - virDomainDef *vmdef,
> - char **jsonstr,
> - size_t *nnicindexes,
> - int **nicindexes)
> +virCHMonitorBuildVMJson(virCHDriver *driver, virDomainDef *vmdef,
> + char **jsonstr)
> {
> g_autoptr(virJSONValue) content = virJSONValueNewObject();
>
> @@ -490,10 +441,6 @@ virCHMonitorBuildVMJson(virCHDriver *driver,
> if (virCHMonitorBuildDisksJson(content, vmdef) < 0)
> return -1;
>
> -
> - if (virCHMonitorBuildNetsJson(content, vmdef, nnicindexes, nicindexes) < 0)
> - return -1;
> -
> if (virCHMonitorBuildDevicesJson(content, vmdef) < 0)
> return -1;
>
> @@ -877,10 +824,7 @@ virCHMonitorShutdownVMM(virCHMonitor *mon)
> }
>
> int
> -virCHMonitorCreateVM(virCHDriver *driver,
> - virCHMonitor *mon,
> - size_t *nnicindexes,
> - int **nicindexes)
> +virCHMonitorCreateVM(virCHDriver *driver, virCHMonitor *mon)
> {
> g_autofree char *url = NULL;
> int responseCode = 0;
> @@ -892,8 +836,7 @@ virCHMonitorCreateVM(virCHDriver *driver,
> headers = curl_slist_append(headers, "Accept: application/json");
> headers = curl_slist_append(headers, "Content-Type: application/json");
>
> - if (virCHMonitorBuildVMJson(driver, mon->vm->def, &payload,
> - nnicindexes, nicindexes) != 0)
> + if (virCHMonitorBuildVMJson(driver, mon->vm->def, &payload) != 0)
> return -1;
>
> VIR_WITH_OBJECT_LOCK_GUARD(mon) {
> diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h
> index bbfa77cdff..47b4e7abbd 100644
> --- a/src/ch/ch_monitor.h
> +++ b/src/ch/ch_monitor.h
> @@ -104,10 +104,7 @@ void virCHMonitorClose(virCHMonitor *mon);
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCHMonitor, virCHMonitorClose);
>
>
> -int virCHMonitorCreateVM(virCHDriver *driver,
> - virCHMonitor *mon,
> - size_t *nnicindexes,
> - int **nicindexes);
> +int virCHMonitorCreateVM(virCHDriver *driver, virCHMonitor *mon);
> int virCHMonitorBootVM(virCHMonitor *mon);
> int virCHMonitorShutdownVM(virCHMonitor *mon);
> int virCHMonitorRebootVM(virCHMonitor *mon);
> @@ -123,3 +120,5 @@ size_t virCHMonitorGetThreadInfo(virCHMonitor *mon, bool refresh,
> virCHMonitorThreadInfo **threads);
> int virCHMonitorGetIOThreads(virCHMonitor *mon,
> virDomainIOThreadInfo ***iothreads);
> +int
> +virCHMonitorBuildNetJson(virDomainNetDef *netdef, char **jsonstr);
> diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c
> index f3bb4a7280..5bf24932d0 100644
> --- a/src/ch/ch_process.c
> +++ b/src/ch/ch_process.c
> @@ -27,9 +27,13 @@
> #include "ch_monitor.h"
> #include "ch_process.h"
> #include "domain_cgroup.h"
> +#include "domain_interface.h"
> #include "virerror.h"
> +#include "virfile.h"
> #include "virjson.h"
> #include "virlog.h"
> +#include "virstring.h"
> +#include "ch_interface.h"
>
> #define VIR_FROM_THIS VIR_FROM_CH
>
> @@ -448,13 +452,148 @@ virCHProcessSetupVcpus(virDomainObj *vm)
> return 0;
> }
>
> +/**
> + * chProcessAddNetworkDevices:
> + * @driver: pointer to ch driver object
> + * @mon: pointer to the monitor object
> + * @vmdef: pointer to domain definition
> + * @nicindexes: returned array of FDs of guest interfaces
> + * @nnicindexes: returned number of network indexes
> + *
> + * Send tap fds to CH process via AddNet api. Capture the network indexes of
> + * guest interfaces in nicindexes.
> + *
> + * Returns 0 on success, -1 on error.
> + */
> +static int
> +chProcessAddNetworkDevices(virCHDriver *driver,
> + virCHMonitor *mon,
> + virDomainDef *vmdef,
> + int **nicindexes,
> + size_t *nnicindexes)
> +{
> + size_t i, j, tapfd_len;
> + int mon_sockfd, http_res, ret;
> + g_autofree int *tapfds = NULL;
> + g_autoptr(virJSONValue) net = NULL;
> + struct sockaddr_un server_addr;
> + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> + g_auto(virBuffer) http_headers = VIR_BUFFER_INITIALIZER;
Couple of unused variables, couple of variables used solely within the
loop below.
BTW: just like there's g_autofree, libvirt has its own VIR_AUTOCLOSE,
which can be used like this:
VIR_AUTOCLOSE mon_sockfd = -1;
...
mon_sockfd = socket(...);
if (mon_sockfd < 0)
return -1;
...
if (some_error)
return -1;
...
return 0;
and just like you'd expect - the mon_sockfd is closed on every return
path and NOT leaked. This way, you can simplify the error path which
then becomes just 'return -1' at which point all 'goto err' can simply
be replaced with that 'return -1'.
BTW2: it's an unwritten rule, well, custom, that 'ret' is used to hold
value that's eventually returned from the function. In this case it's
used to hold a temporary value - retval from a called function. In such
cases we use anything else, but 'ret'. 'rc' is my favorite.
> +
> + if (!virBitmapIsBitSet(driver->chCaps, CH_MULTIFD_IN_ADDNET)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Guest networking is not supported by this version of ch"));
> + return -1;
> + }
> +
> + mon_sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
> + if (mon_sockfd < 0) {
> + virReportSystemError(errno, "%s", _("Failed to open a UNIX socket"));
> + return -1;
> + }
> +
> + memset(&server_addr, 0, sizeof(server_addr));
> + server_addr.sun_family = AF_UNIX;
> + if (virStrcpyStatic(server_addr.sun_path, mon->socketpath) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("UNIX socket path '%1$s' too long"), mon->socketpath);
> + goto err;
> + }
> +
> + if (connect(mon_sockfd, (struct sockaddr *)&server_addr,
> + sizeof(server_addr)) == -1) {
> + virReportSystemError(errno, "%s", _("Failed to connect to mon socket"));
> + goto err;
> + }
> +
> + virBufferAddLit(&http_headers, "PUT /api/v1/vm.add-net HTTP/1.1\r\n");
> + virBufferAddLit(&http_headers, "Host: localhost\r\n");
> + virBufferAddLit(&http_headers, "Content-Type: application/json\r\n");
> +
> + for (i = 0; i < vmdef->nnets; i++) {
> + g_autofree char *payload = NULL;
> + g_autofree char *response = NULL;
> +
> + if (vmdef->nets[i]->driver.virtio.queues == 0) {
> + /* "queues" here refers to queue pairs. When 0, initialize
> + * queue pairs to 1.
> + */
> + vmdef->nets[i]->driver.virtio.queues = 1;
> + }
> + tapfd_len = vmdef->nets[i]->driver.virtio.queues;
> +
> + if (virCHDomainValidateActualNetDef(vmdef->nets[i]) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("net definition failed validation"));
> + goto err;
> + }
> +
> + tapfds = g_new0(int, tapfd_len);
> + memset(tapfds, -1, (tapfd_len) * sizeof(int));
> +
> + /* Connect Guest interfaces */
> + if (virCHConnetNetworkInterfaces(driver, vmdef, vmdef->nets[i], tapfds,
> + nicindexes, nnicindexes) < 0)
> + goto err;
> +
> + if (virCHMonitorBuildNetJson(vmdef->nets[i], &payload) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to build net json"));
> + goto err;
> + }
> +
> + VIR_DEBUG("payload sent with net-add request to CH = %s", payload);
> +
> + virBufferAsprintf(&buf, "%s", virBufferCurrentContent(&http_headers));
> + virBufferAsprintf(&buf, "Content-Length: %ld\r\n\r\n", strlen(payload));
> + virBufferAsprintf(&buf, "%s", payload);
> + payload = virBufferContentAndReset(&buf);
> +
> + if (virSocketSendMsgWithFDs(mon_sockfd, payload, tapfds, tapfd_len) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to send net-add request to CH"));
> + goto err;
> + }
> +
Ooops, so if sending FDs fails for whatever reason, tapfds[] are not closed.
> + /* Close sent tap fds in Libvirt, as they have been dup()ed in CH */
> + for (j = 0; j < tapfd_len; j++) {
> + VIR_FORCE_CLOSE(tapfds[j]);
> + }
> +
> + /* Process the response from CH */
> + response = virSocketRecv(mon_sockfd);
> + if (response == NULL) {
> + VIR_ERROR(_("Failed while receiving response from CH"));
No, VIR_ERROR() does not do what you think it does. It shouldn't really
be used anywhere, but a brief moment in daemon startup when loggin is
not set up yet. Here, virSocketRecv() reports an error in case of NULL,
but ...
> + goto err;
> + }
> +
> + /* Parse the HTTP response code */
> + ret = sscanf(response, "HTTP/1.%*d %d", &http_res);
> + if (ret != 1) {
> + VIR_ERROR(_("Failed to parse HTTP response code"));
> + goto err;
.. here, if this error is met then control jumps onto err label, this
error message is printed only into the logs but not reported to the
caller of API. For instance, if this was called via 'virsh start', then
user would see:
# virsh start myCHGuest
Failed to start domain: 'myCHGuest'
An error occurred, but the cause is unknown
and would need to open logs only to find what went wrong.
> + }
> + if (http_res != 204 && http_res != 200) {
> + VIR_ERROR(_("Unexpected response from CH: %1$d"), http_res);
> + goto err;
> + }
> + }
> +
> + VIR_FORCE_CLOSE(mon_sockfd);
> + return 0;
> +
> + err:
> + VIR_FORCE_CLOSE(mon_sockfd);
> + return -1;
> +}
> +
Michal
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
