Pass "net_<index>" as net id to CH. This is to have better control over the network configs. This id can be further used in performing operations like restore etc. Signed-off-by: Purna Pavan Chandra <paekkaladevi(a)linux.microsoft.com&gt; --- src/ch/ch_monitor.c | 7 ++++++- src/ch/ch_monitor.h | 4 +++- src/ch/ch_process.c | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 7b6b77de1c..6af5b29d82 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -278,18 +278,23 @@ virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef) /** * virCHMonitorBuildNetJson: * @net: pointer to a guest network definition + * @netindex: index of the 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) +virCHMonitorBuildNetJson(virDomainNetDef *net, int netindex, char **jsonstr) { char macaddr[VIR_MAC_STRING_BUFLEN]; g_autoptr(virJSONValue) net_json = virJSONValueNewObject(); virDomainNetType actualType = virDomainNetGetActualType(net); + g_autofree char *id = g_strdup_printf("%s_%d", CH_NET_ID_PREFIX, netindex); + if (virJSONValueObjectAppendString(net_json, "id", id) < 0) + return -1; + if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET && net->guestIP.nips == 1) { const virNetDevIPAddr *ip; diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h index ea6b2a771b..bac80b5fcb 100644 --- a/src/ch/ch_monitor.h +++ b/src/ch/ch_monitor.h @@ -42,6 +42,8 @@ #define VIRCH_THREAD_NAME_LEN 16 +#define CH_NET_ID_PREFIX "net" + typedef enum { virCHThreadTypeEmulator, virCHThreadTypeVcpu, @@ -125,4 +127,4 @@ size_t virCHMonitorGetThreadInfo(virCHMonitor *mon, bool refresh, int virCHMonitorGetIOThreads(virCHMonitor *mon, virDomainIOThreadInfo ***iothreads); int -virCHMonitorBuildNetJson(virDomainNetDef *netdef, char **jsonstr); +virCHMonitorBuildNetJson(virDomainNetDef *netdef, int netindex, char **jsonstr); diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 6f99fec900..5907a6e81f 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -621,7 +621,7 @@ chProcessAddNetworkDevices(virCHDriver *driver, nicindexes, nnicindexes) < 0) return -1; - if (virCHMonitorBuildNetJson(vmdef->nets[i], &payload) < 0) { + if (virCHMonitorBuildNetJson(vmdef->nets[i], i, &payload) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to build net json")); return -1; -- 2.34.1

chSocketRecv fn can be used by operations such as restore, which cannot have a specific poll timeout. The runtime of these operations at server side (vmm) cannot be determined or capped as it depends on the guest configuration. Hence, add a new parameter 'use_timeout' which when set will pass -1 as timeout to poll, otherwise the default PKT_TIMEOUT_MS is used. Signed-off-by: Purna Pavan Chandra <paekkaladevi(a)linux.microsoft.com&gt; --- src/ch/ch_process.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 1943c4381b..851ab7545c 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -534,11 +534,12 @@ chMonitorSocketConnect(virCHMonitor *mon) #define PKT_TIMEOUT_MS 500 /* ms */ static char * -chSocketRecv(int sock) +chSocketRecv(int sock, bool use_timeout) { struct pollfd pfds[1]; char *buf = NULL; size_t buf_len = 1024; + int timeout = PKT_TIMEOUT_MS; int ret; buf = g_new0(char, buf_len); @@ -546,8 +547,11 @@ chSocketRecv(int sock) pfds[0].fd = sock; pfds[0].events = POLLIN; + if (!use_timeout) + timeout = -1; + do { - ret = poll(pfds, G_N_ELEMENTS(pfds), PKT_TIMEOUT_MS); + ret = poll(pfds, G_N_ELEMENTS(pfds), timeout); } while (ret < 0 && errno == EINTR); if (ret <= 0) { @@ -574,12 +578,12 @@ chSocketRecv(int sock) #undef PKT_TIMEOUT_MS static int -chSocketProcessHttpResponse(int sock) +chSocketProcessHttpResponse(int sock, bool use_poll_timeout) { g_autofree char *response = NULL; int http_res; - response = chSocketRecv(sock); + response = chSocketRecv(sock, use_poll_timeout); if (response == NULL) { return -1; } @@ -705,7 +709,7 @@ chProcessAddNetworkDevices(virCHDriver *driver, return -1; } - if (chSocketProcessHttpResponse(mon_sockfd) < 0) + if (chSocketProcessHttpResponse(mon_sockfd, true) < 0) return -1; } -- 2.34.1

On 7/4/2024 6:13 AM, Purna Pavan Chandra wrote: typos: s/syncronous/synchronous/ s/operationin/operation in/ s/reponse/response -- Regards, Praveen

Cloud-hypervisor now supports restoring with new net fds. Ref: https://github.com/cloud-hypervisor/cloud-hypervisor/pull/6402 So, pass new tap fds via SCM_RIGHTS to CH's restore api. Signed-off-by: Purna Pavan Chandra <paekkaladevi(a)linux.microsoft.com&gt; --- src/ch/ch_capabilities.c | 6 +++ src/ch/ch_capabilities.h | 1 + src/ch/ch_driver.c | 29 +++++++----- src/ch/ch_monitor.c | 21 ++++++++- src/ch/ch_monitor.h | 2 +- src/ch/ch_process.c | 98 +++++++++++++++++++++++++++++++++------- 6 files changed, 126 insertions(+), 31 deletions(-) diff --git a/src/ch/ch_capabilities.c b/src/ch/ch_capabilities.c index 5941851500..f3765a8095 100644 --- a/src/ch/ch_capabilities.c +++ b/src/ch/ch_capabilities.c @@ -65,6 +65,12 @@ virCHCapsInitCHVersionCaps(int version) if (version >= 36000000) virCHCapsSet(chCaps, CH_SOCKET_BACKEND_SERIAL_PORT); + /* Starting v40, Cloud-Hypervisor supports restore with new net fds. + * This is required to be able to restore a guest with network config define. + * https://github.com/cloud-hypervisor/cloud-hypervisor/releases/tag/v40.0 */ + if (version >= 40000000) + virCHCapsSet(chCaps, CH_RESTORE_WITH_NEW_TAPFDS); + return g_steal_pointer(&chCaps); } diff --git a/src/ch/ch_capabilities.h b/src/ch/ch_capabilities.h index 03932511f6..e5efb14b68 100644 --- a/src/ch/ch_capabilities.h +++ b/src/ch/ch_capabilities.h @@ -28,6 +28,7 @@ typedef enum { CH_SERIAL_CONSOLE_IN_PARALLEL, /* Serial and Console ports can work in parallel */ CH_MULTIFD_IN_ADDNET, /* Cloud-hypervisor can accept multiple FDs in add-net api */ CH_SOCKET_BACKEND_SERIAL_PORT, /* Support Unix socket as a backend for a serial port */ + CH_RESTORE_WITH_NEW_TAPFDS, /* Cloud-hypervisor support for restore with new net fds */ CH_CAPS_LAST /* this must always be the last item */ } virCHCapsFlags; diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index fbeac60825..24aed471c5 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -680,22 +680,24 @@ chDomainDestroy(virDomainPtr dom) } static int -chDomainSaveAdditionalValidation(virDomainDef *vmdef) +chDomainSaveRestoreAdditionalValidation(virCHDriver *driver, virDomainDef *vmdef) { - /* - SAVE and RESTORE are functional only without any networking and - device passthrough configuration - */ - if (vmdef->nnets > 0) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot save domain with network interfaces")); - return -1; - } + /* SAVE and RESTORE are functional only without any host device + * passthrough configuration */ if (vmdef->nhostdevs > 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot save domain with host devices")); + _("cannot save/restore domain with host devices")); return -1; } + + if (vmdef->nnets > 0) { + if (!virBitmapIsBitSet(driver->chCaps, CH_RESTORE_WITH_NEW_TAPFDS)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot save/restore domain with network devices")); + return -1; + } + } + return 0; } @@ -728,7 +730,7 @@ chDoDomainSave(virCHDriver *driver, VIR_AUTOCLOSE fd = -1; int ret = -1; - if (chDomainSaveAdditionalValidation(vm->def) < 0) + if (chDomainSaveRestoreAdditionalValidation(driver, vm->def) < 0) goto end; domainState = virDomainObjGetState(vm, NULL); @@ -1087,6 +1089,9 @@ chDomainRestoreFlags(virConnectPtr conn, if (virDomainRestoreFlagsEnsureACL(conn, def) < 0) goto cleanup; + if (chDomainSaveRestoreAdditionalValidation(driver, def) < 0) + goto cleanup; + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 8d8be1f46e..3af5e7f2d1 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -978,14 +978,31 @@ virCHMonitorSaveVM(virCHMonitor *mon, const char *to) } int -virCHMonitorBuildRestoreJson(const char *from, char **jsonstr) +virCHMonitorBuildRestoreJson(virDomainDef *vmdef, const char *from, char **jsonstr) { + size_t i; g_autoptr(virJSONValue) restore_json = virJSONValueNewObject(); - g_autofree char *path_url = g_strdup_printf("file://%s", from); + if (virJSONValueObjectAppendString(restore_json, "source_url", path_url)) return -1; + /* Pass the netconfig needed to restore with new netfds */ + if (vmdef->nnets) { + g_autoptr(virJSONValue) nets = virJSONValueNewArray(); + for (i = 0; i < vmdef->nnets; i++) { + g_autoptr(virJSONValue) net_json = virJSONValueNewObject(); + g_autofree char *id = g_strdup_printf("%s_%ld", CH_NET_ID_PREFIX, i); + if (virJSONValueObjectAppendString(net_json, "id", id) < 0) + return -1; + if (virJSONValueObjectAppendNumberInt(net_json, "num_fds", vmdef->nets[i]->driver.virtio.queues)) + return -1; + virJSONValueArrayAppend(nets, &net_json); + } + if (virJSONValueObjectAppend(restore_json, "net_fds", &nets)) + return -1; + } + if (!(*jsonstr = virJSONValueToString(restore_json, false))) return -1; diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h index 375b7a49a4..02a6685d58 100644 --- a/src/ch/ch_monitor.h +++ b/src/ch/ch_monitor.h @@ -127,4 +127,4 @@ int virCHMonitorGetIOThreads(virCHMonitor *mon, virDomainIOThreadInfo ***iothreads); int virCHMonitorBuildNetJson(virDomainNetDef *netdef, int netindex, char **jsonstr); -int virCHMonitorBuildRestoreJson(const char *from, char **jsonstr); +int virCHMonitorBuildRestoreJson(virDomainDef *vmdef, const char *from, char **jsonstr); diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 0ac0a5c9a2..540bf6801d 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -29,6 +29,7 @@ #include "ch_process.h" #include "domain_cgroup.h" #include "domain_interface.h" +#include "viralloc.h" #include "virerror.h" #include "virfile.h" #include "virjson.h" @@ -716,6 +717,56 @@ chProcessAddNetworkDevices(virCHDriver *driver, return 0; } +/** + * virCHRestoreCreateNetworkDevices: + * @driver: pointer to driver structure + * @vmdef: pointer to domain definition + * @vmtapfds: returned array of FDs of guest interfaces + * @nvmtapfds: returned number of network indexes + * @nicindexes: returned array of network indexes + * @nnicindexes: returned number of network indexes + * + * Create network devices for the domain. This function is called during + * domain restore. + * + * Returns 0 on success or -1 in case of error +*/ +static int +virCHRestoreCreateNetworkDevices(virCHDriver *driver, + virDomainDef *vmdef, + int **vmtapfds, + size_t *nvmtapfds, + int **nicindexes, + size_t *nnicindexes) +{ + size_t i, j; + size_t tapfd_len; + size_t index_vmtapfds; + for (i = 0; i < vmdef->nnets; i++) { + g_autofree int *tapfds = NULL; + tapfd_len = vmdef->nets[i]->driver.virtio.queues; + if (virCHDomainValidateActualNetDef(vmdef->nets[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("net definition failed validation")); + return -1; + } + 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) + return -1; + + index_vmtapfds = *nvmtapfds; + VIR_EXPAND_N(*vmtapfds, *nvmtapfds, tapfd_len); + for (j = 0; j < tapfd_len; j++) { + VIR_APPEND_ELEMENT_INPLACE(*vmtapfds, index_vmtapfds, tapfds[j]); + } + } + return 0; +} + /** * virCHProcessStartValidate: * @driver: pointer to driver structure @@ -917,7 +968,12 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from g_autofree char *payload = NULL; g_autofree char *response = NULL; VIR_AUTOCLOSE mon_sockfd = -1; + g_autofree int *tapfds = NULL; + g_autofree int *nicindexes = NULL; size_t payload_len; + size_t ntapfds = 0; + size_t nnicindexes = 0; + int ret = -1; if (!priv->monitor) { /* Get the first monitor connection if not already */ @@ -932,17 +988,7 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from vm->def->id = vm->pid; priv->machineName = virCHDomainGetMachineName(vm); - /* Pass 0, NULL as restore only works without networking support */ - if (virDomainCgroupSetupCgroup("ch", vm, - 0, NULL, /* nnicindexes, nicindexes */ - &priv->cgroup, - cfg->cgroupControllers, - 0, /*maxThreadsPerProc*/ - priv->driver->privileged, - priv->machineName) < 0) - return -1; - - if (virCHMonitorBuildRestoreJson(from, &payload) < 0) { + if (virCHMonitorBuildRestoreJson(vm->def, from, &payload) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to restore domain")); return -1; @@ -960,20 +1006,40 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from if ((mon_sockfd = chMonitorSocketConnect(priv->monitor)) < 0) return -1; - if (virSocketSendMsgWithFDs(mon_sockfd, payload, payload_len, NULL, 0) < 0) { + if (virCHRestoreCreateNetworkDevices(driver, vm->def, &tapfds, &ntapfds, &nicindexes, &nnicindexes) < 0) + goto cleanup; + + if (virDomainCgroupSetupCgroup("ch", vm, + nnicindexes, nicindexes, + &priv->cgroup, + cfg->cgroupControllers, + 0, /*maxThreadsPerProc*/ + priv->driver->privileged, + priv->machineName) < 0) + goto cleanup; + + /* Bring up netdevs before restoring vm */ + if (virDomainInterfaceStartDevices(vm->def) < 0) + goto cleanup; + + if (virSocketSendMsgWithFDs(mon_sockfd, payload, payload_len, tapfds, ntapfds) < 0) { virReportSystemError(errno, "%s", _("Failed to send restore request to CH")); - return -1; + goto cleanup; } /* Restore is a syncronous operationin CH. so, pass false to wait until there's a reponse */ if (chSocketProcessHttpResponse(mon_sockfd, false) < 0) - return -1; + goto cleanup; if (virCHProcessSetup(vm) < 0) - return -1; + goto cleanup; virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); + ret = 0; - return 0; + cleanup: + if (tapfds) + chCloseFDs(tapfds, ntapfds); + return ret; } -- 2.34.1

On Thu, Jul 25, 2024 at 12:04:47PM -0500, Praveen K Paladugu wrote: chProcessAddNetworkDevices is already refactored and the common code is moved to new functions. Since the action performed inside the loop are different here, this can not be refactored further. This is taken care in the cleanup section. > > /* Restore is a syncronous operationin CH. so, pass false to wait until there's a reponse */ > > if (chSocketProcessHttpResponse(mon_sockfd, false) < 0) > > - return -1; > > + goto cleanup; > > if (virCHProcessSetup(vm) < 0) > > - return -1; > > + goto cleanup; > > virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); > > + ret = 0; > > - return 0; > > + cleanup: > > + if (tapfds) > > + chCloseFDs(tapfds, ntapfds); > > + return ret; > > } > > -- > Regards, > Praveen