On 10/22/21 5:37 PM, Praveen K Paladugu wrote:
> From: Vineeth Pillai <viremana@xxxxxxxxxxxxxxxxxxx>
>
> Signed-off-by: Vineeth Pillai <viremana@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Praveen Paladugu <prapal@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Praveen K Paladugu <prapal@xxxxxxxxxxxxxxxxxxx>
> ---
> src/ch/ch_conf.h | 5 +++
> src/ch/ch_domain.c | 26 +++++++++++++-
> src/ch/ch_domain.h | 4 +--
> src/ch/ch_driver.c | 4 ++-
> src/ch/ch_monitor.c | 85 +++++++++++++++++++++++++++++++++++++++++----
> src/ch/ch_monitor.h | 14 +++++++-
> src/ch/ch_process.c | 6 +++-
> src/ch/meson.build | 1 +
> 8 files changed, 132 insertions(+), 13 deletions(-)
>
> diff --git a/src/ch/ch_conf.h b/src/ch/ch_conf.h
> index 37c36d9a09..49f286f97a 100644
> --- a/src/ch/ch_conf.h
> +++ b/src/ch/ch_conf.h
> @@ -44,6 +44,11 @@ struct _virCHDriver
> {
> virMutex lock;
>
> + bool privileged;
> +
> + /* Embedded Mode: Not yet supported */
> + char *embeddedRoot;
> +
> /* Require lock to get a reference on the object,
> * lockless access thereafter */
> virCaps *caps;
> diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
> index c0b0b1005a..e1030800aa 100644
> --- a/src/ch/ch_domain.c
> +++ b/src/ch/ch_domain.c
> @@ -21,10 +21,12 @@
> #include <config.h>
>
> #include "ch_domain.h"
> +#include "domain_driver.h"
> #include "viralloc.h"
> #include "virchrdev.h"
> #include "virlog.h"
> #include "virtime.h"
> +#include "virsystemd.h"
>
> #define VIR_FROM_THIS VIR_FROM_CH
>
> @@ -136,7 +138,7 @@ virCHDomainObjEndJob(virDomainObj *obj)
> }
>
> static void *
> -virCHDomainObjPrivateAlloc(void *opaque G_GNUC_UNUSED)
> +virCHDomainObjPrivateAlloc(void *opaque)
> {
> virCHDomainObjPrivate *priv;
>
> @@ -152,6 +154,7 @@ virCHDomainObjPrivateAlloc(void *opaque G_GNUC_UNUSED)
> g_free(priv);
> return NULL;
> }
> + priv->driver = opaque;
>
> return priv;
> }
> @@ -363,3 +366,24 @@ virCHDomainHasVcpuPids(virDomainObj *vm)
>
> return false;
> }
> +
> +char *virCHDomainGetMachineName(virDomainObj *vm)
> +{
> + virCHDomainObjPrivate *priv = CH_DOMAIN_PRIVATE(vm);
> + virCHDriver *driver = priv->driver;
> + char *ret = NULL;
> +
> + if (vm->pid > 0) {
> + ret = virSystemdGetMachineNameByPID(vm->pid);
> + if (!ret)
> + virResetLastError();
> + }
> +
> + if (!ret)
> + ret = virDomainDriverGenerateMachineName("ch",
> + driver->embeddedRoot,
> + vm->def->id, vm->def->name,
> + driver->privileged);
> +
> + return ret;
> +}
> diff --git a/src/ch/ch_domain.h b/src/ch/ch_domain.h
> index e35777a9ec..3ac3421015 100644
> --- a/src/ch/ch_domain.h
> +++ b/src/ch/ch_domain.h
> @@ -54,9 +54,7 @@ struct _virCHDomainObjPrivate {
> struct virCHDomainJobObj job;
>
> virChrdevs *chrdevs;
> -
> virCgroup *cgroup;
> -
These extra spaces were introduced just a few patches back. Could you
change those so that you avoid removing them here?
> virCHDriver *driver;
> virCHMonitor *monitor;
> char *machineName;
> @@ -94,3 +92,5 @@ virCHDomainObjEndJob(virDomainObj *obj);
> int virCHDomainRefreshVcpuInfo(virDomainObj *vm);
> pid_t virCHDomainGetVcpuPid(virDomainObj *vm, unsigned int vcpuid);
> bool virCHDomainHasVcpuPids(virDomainObj *vm);
> +
> +char *virCHDomainGetMachineName(virDomainObj *vm);
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 62ca6c1994..ca854da123 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -944,7 +944,9 @@ static int chStateInitialize(bool privileged,
> goto cleanup;
> }
>
> - ret = VIR_DRV_STATE_INIT_COMPLETE;
> + ch_driver->privileged = privileged;
> +
> + return VIR_DRV_STATE_INIT_COMPLETE;
Storing @privileged is okay, but changing ret to return doesn't feel
right. If you really want to do that then rename cleanup to error and
drop if() there since the condition will always be true (keep the body
thogh!). Or just don't change ret to return.
>
> cleanup:
> if (ret != VIR_DRV_STATE_INIT_COMPLETE)
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index 691d1ce64b..c0ae031200 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -228,7 +228,8 @@ virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef)
> }
>
> static int
> -virCHMonitorBuildNetJson(virJSONValue *nets, virDomainNetDef *netdef)
> +virCHMonitorBuildNetJson(virJSONValue *nets, virDomainNetDef *netdef,
> + size_t *nnicindexes, int **nicindexes)
> {
> virDomainNetType netType = virDomainNetGetActualType(netdef);
> char macaddr[VIR_MAC_STRING_BUFLEN];
> @@ -263,6 +264,18 @@ virCHMonitorBuildNetJson(virJSONValue *nets, virDomainNetDef *netdef)
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("ethernet type supports a single guest ip"));
> }
> +
> + /* 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;
> + else
In these patterns we don't really use else. Replace it wit an empty line
please so that ..
> + VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex);
.. this line can use one indentation level less.
> + }
> +
> break;
> case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> if ((virDomainChrType)netdef->data.vhostuser->type != VIR_DOMAIN_CHR_TYPE_UNIX) {
> @@ -331,7 +344,8 @@ virCHMonitorBuildNetJson(virJSONValue *nets, virDomainNetDef *netdef)
> }
>
> static int
> -virCHMonitorBuildNetsJson(virJSONValue *content, virDomainDef *vmdef)
> +virCHMonitorBuildNetsJson(virJSONValue *content, virDomainDef *vmdef,
> + size_t *nnicindexes, int **nicindexes)
> {
> g_autoptr(virJSONValue) nets = NULL;
> size_t i;
> @@ -340,7 +354,8 @@ virCHMonitorBuildNetsJson(virJSONValue *content, virDomainDef *vmdef)
> nets = virJSONValueNewArray();
>
> for (i = 0; i < vmdef->nnets; i++) {
> - if (virCHMonitorBuildNetJson(nets, vmdef->nets[i]) < 0)
> + if (virCHMonitorBuildNetJson(nets, vmdef->nets[i],
> + nnicindexes, nicindexes) < 0)
> return -1;
> }
> if (virJSONValueObjectAppend(content, "net", &nets) < 0)
> @@ -351,7 +366,57 @@ virCHMonitorBuildNetsJson(virJSONValue *content, virDomainDef *vmdef)
> }
>
> static int
> -virCHMonitorBuildVMJson(virDomainDef *vmdef, char **jsonstr)
> +virCHMonitorBuildDeviceJson(virJSONValue *devices, virDomainHostdevDef *hostdevdef)
> +{
> + g_autoptr(virJSONValue) device = NULL;
> +
> +
> + if (hostdevdef->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> + hostdevdef->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
> + g_autofree char *name = NULL;
> + g_autofree char *path = NULL;
> + virDomainHostdevSubsysPCI *pcisrc = &hostdevdef->source.subsys.u.pci;
> + device = virJSONValueNewObject();
> + name = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, pcisrc->addr.domain,
> + pcisrc->addr.bus, pcisrc->addr.slot,
> + pcisrc->addr.function);
Please separate declaration and code blocks with one empty line. It is
easier to read that way.
Also, please consider using virPCIDeviceAddressAsString().
> + path = g_strdup_printf("/sys/bus/pci/devices/%s/", name);
> + if (!virFileExists(path)) {
> + virReportError(VIR_ERR_DEVICE_MISSING,
> + _("host pci device %s not found"), path);
> + return -1;
> + }
> + if (virJSONValueObjectAppendString(device, "path", path) < 0)
> + return -1;
> + if (virJSONValueArrayAppend(devices, &device) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +virCHMonitorBuildDevicesJson(virJSONValue *content, virDomainDef *vmdef)
> +{
> + g_autoptr(virJSONValue) devices = NULL;
This variable should be declared in the if() body below. Alternatively,
the condition can be reversed and the function can return early.
Something like this:
if (vmdef->nhostdevs == 0)
return 0;
devices = virJSONValueNewArray();
for(...) ...
> + size_t i;
> +
> + if (vmdef->nhostdevs > 0) {
> + devices = virJSONValueNewArray();
> + for (i = 0; i < vmdef->nhostdevs; i++) {
> + if (virCHMonitorBuildDeviceJson(devices, vmdef->hostdevs[i]) < 0)
> + return -1;
> + }
> + if (virJSONValueObjectAppend(content, "devices", &devices) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +virCHMonitorBuildVMJson(virDomainDef *vmdef, char **jsonstr,
> + size_t *nnicindexes, int **nicindexes)
> {
> g_autoptr(virJSONValue) content = virJSONValueNewObject();
>
> @@ -376,7 +441,12 @@ virCHMonitorBuildVMJson(virDomainDef *vmdef, char **jsonstr)
> if (virCHMonitorBuildDisksJson(content, vmdef) < 0)
> return -1;
>
> - if (virCHMonitorBuildNetsJson(content, vmdef) < 0)
> +
> + if (virCHMonitorBuildNetsJson(content, vmdef,
> + nnicindexes, nicindexes) < 0)
> + return -1;
> +
> + if (virCHMonitorBuildDevicesJson(content, vmdef) < 0)
> return -1;
>
> if (!(*jsonstr = virJSONValueToString(content, false)))
> @@ -673,7 +743,7 @@ virCHMonitorShutdownVMM(virCHMonitor *mon)
> }
>
> int
> -virCHMonitorCreateVM(virCHMonitor *mon)
> +virCHMonitorCreateVM(virCHMonitor *mon, size_t *nnicindexes, int **nicindexes)
> {
> g_autofree char *url = NULL;
> int responseCode = 0;
> @@ -685,7 +755,8 @@ virCHMonitorCreateVM(virCHMonitor *mon)
> headers = curl_slist_append(headers, "Accept: application/json");
> headers = curl_slist_append(headers, "Content-Type: application/json");
>
> - if (virCHMonitorBuildVMJson(mon->vm->def, &payload) != 0)
> + if (virCHMonitorBuildVMJson(mon->vm->def, &payload,
> + nnicindexes, nicindexes) != 0)
> return -1;
>
> virObjectLock(mon);
> diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h
> index 0f684ca583..8ca9e17a9a 100644
> --- a/src/ch/ch_monitor.h
> +++ b/src/ch/ch_monitor.h
> @@ -55,10 +55,22 @@ virCHMonitor *virCHMonitorNew(virDomainObj *vm, const char *socketdir);
> void virCHMonitorClose(virCHMonitor *mon);
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCHMonitor, virCHMonitorClose);
>
> -int virCHMonitorCreateVM(virCHMonitor *mon);
> +
> +int virCHMonitorCreateVM(virCHMonitor *mon,
> + size_t *nnicindexes, int **nicindexes);
> int virCHMonitorBootVM(virCHMonitor *mon);
> int virCHMonitorShutdownVM(virCHMonitor *mon);
> int virCHMonitorRebootVM(virCHMonitor *mon);
> int virCHMonitorSuspendVM(virCHMonitor *mon);
> int virCHMonitorResumeVM(virCHMonitor *mon);
> int virCHMonitorGetInfo(virCHMonitor *mon, virJSONValue **info);
> +
> +typedef struct _virCHMonitorCPUInfo virCHMonitorCPUInfo;
> +struct _virCHMonitorCPUInfo {
> + pid_t tid;
> + bool online;
> +};
> +void virCHMonitorCPUInfoFree(virCHMonitorCPUInfo *cpus);
> +int virCHMonitorGetCPUInfo(virCHMonitor *mon,
> + virCHMonitorCPUInfo **vcpus,
> + size_t maxvcpus);
> diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c
> index 1a01ca9384..3b7f6fcddf 100644
> --- a/src/ch/ch_process.c
> +++ b/src/ch/ch_process.c
> @@ -149,6 +149,8 @@ int virCHProcessStart(virCHDriver *driver,
> {
> int ret = -1;
> virCHDomainObjPrivate *priv = vm->privateData;
> + g_autofree int *nicindexes = NULL;
> + size_t nnicindexes = 0;
>
> if (!priv->monitor) {
> /* And we can get the first monitor connection now too */
> @@ -158,7 +160,8 @@ int virCHProcessStart(virCHDriver *driver,
> goto cleanup;
> }
>
> - if (virCHMonitorCreateVM(priv->monitor) < 0) {
> + if (virCHMonitorCreateVM(priv->monitor,
> + &nnicindexes, &nicindexes) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("failed to create guest VM"));
> goto cleanup;
> @@ -171,6 +174,7 @@ int virCHProcessStart(virCHDriver *driver,
> goto cleanup;
> }
>
> + priv->machineName = virCHDomainGetMachineName(vm);
> vm->pid = priv->monitor->pid;
> vm->def->id = vm->pid;
>
> diff --git a/src/ch/meson.build b/src/ch/meson.build
> index 5c6cab2a9f..2b2bdda26c 100644
> --- a/src/ch/meson.build
> +++ b/src/ch/meson.build
> @@ -29,6 +29,7 @@ if conf.has('WITH_CH')
> ],
> include_directories: [
> conf_inc_dir,
> + hypervisor_inc_dir,
> ],
> link_with: [
> virt_util_lib,
>
Michal
