[PATCH v3 00/13] CH: Add disk hotplug support to Cloud Hypervisor domains
This patchset adds the necessary functionality to support disk hotplugging in the CH driver. Some alias handling that can be extended to other device types is added, in order to allow detaching the devices via the Cloud Hypervisor API. Stefan Kober (13): ch: add ch_hotplug.{h,c} files to CH build ch: pass disk alias to CHV ch: add ch_alias.{c,h} for device alias handling ch: refactor virCHMonitorBuildDiskJson ch: add/use virCHMonitorPut function ch: add monitor disk attach logic ch: add disk attach helper functions ch: implement disk attach in public API ch: assign aliases in ProcessPrepareDomain ch: add virCHMonitorBuildKeyValueJson ch: add virCHMonitorRemoveDevice function ch: implement disk device detach in public API NEWS: announce disk hotplug support for ch NEWS.rst | 5 + po/POTFILES | 2 + src/ch/ch_alias.c | 62 +++++++++ src/ch/ch_alias.h | 27 ++++ src/ch/ch_driver.c | 86 ++++++++++++ src/ch/ch_hotplug.c | 333 ++++++++++++++++++++++++++++++++++++++++++++ src/ch/ch_hotplug.h | 33 +++++ src/ch/ch_monitor.c | 131 ++++++++++++++--- src/ch/ch_monitor.h | 7 + src/ch/ch_process.c | 4 + src/ch/meson.build | 4 + 11 files changed, 671 insertions(+), 23 deletions(-) create mode 100644 src/ch/ch_alias.c create mode 100644 src/ch/ch_alias.h create mode 100644 src/ch/ch_hotplug.c create mode 100644 src/ch/ch_hotplug.h -- 2.50.1
The files are meant to contain all device hotplug related code. The first implementation will be live storage attach and detach. On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_hotplug.c | 35 +++++++++++++++++++++++++++++++++++ src/ch/ch_hotplug.h | 27 +++++++++++++++++++++++++++ src/ch/meson.build | 2 ++ 3 files changed, 64 insertions(+) create mode 100644 src/ch/ch_hotplug.c create mode 100644 src/ch/ch_hotplug.h diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c new file mode 100644 index 0000000000..c46628e7e9 --- /dev/null +++ b/src/ch/ch_hotplug.c @@ -0,0 +1,35 @@ +/* + * ch_hotplug.c: CH device hotplug handling + * + * 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 "ch_hotplug.h" + +#define VIR_FROM_THIS VIR_FROM_CH + +int +chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm G_GNUC_UNUSED, + virCHDriver *driver G_GNUC_UNUSED, + const char *xml G_GNUC_UNUSED, + unsigned int flags) +{ + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + return -1; +} diff --git a/src/ch/ch_hotplug.h b/src/ch/ch_hotplug.h new file mode 100644 index 0000000000..04915ba5de --- /dev/null +++ b/src/ch/ch_hotplug.h @@ -0,0 +1,27 @@ +/* + * ch_hotplug.h: CH device hotplug handling + * + * 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" + +int +chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm, + virCHDriver *driver, + const char *xml, + unsigned int flags); diff --git a/src/ch/meson.build b/src/ch/meson.build index cd20c3d065..bba7ee90ee 100644 --- a/src/ch/meson.build +++ b/src/ch/meson.build @@ -17,6 +17,8 @@ ch_driver_sources = [ 'ch_process.h', 'ch_hostdev.c', 'ch_hostdev.h', + 'ch_hotplug.c', + 'ch_hotplug.h', ] driver_source_files += files(ch_driver_sources) -- 2.50.1
On 9/4/25 14:10, Stefan Kober wrote:
The files are meant to contain all device hotplug related code. The first implementation will be live storage attach and detach.
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_hotplug.c | 35 +++++++++++++++++++++++++++++++++++ src/ch/ch_hotplug.h | 27 +++++++++++++++++++++++++++ src/ch/meson.build | 2 ++ 3 files changed, 64 insertions(+) create mode 100644 src/ch/ch_hotplug.c create mode 100644 src/ch/ch_hotplug.h
This could have been merged with one of patches that actually fills the file with useful code. But at this point I'm too lazy to fix that O:-) Michal
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_monitor.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 6bf877fef3..d369236183 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -249,6 +249,11 @@ virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef) _("Missing disk file path in domain")); return -1; } + if (!diskdef->info.alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing disk alias")); + return -1; + } if (diskdef->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { virReportError(VIR_ERR_INVALID_ARG, _("Only virtio bus types are supported for '%1$s'"), @@ -261,6 +266,9 @@ virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef) if (virJSONValueObjectAppendBoolean(disk, "readonly", true) < 0) return -1; } + if (virJSONValueObjectAppendString(disk, "id", diskdef->info.alias) < 0) { + return -1; + } if (virJSONValueArrayAppend(disks, &disk) < 0) return -1; -- 2.50.1
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- po/POTFILES | 1 + src/ch/ch_alias.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++ src/ch/ch_alias.h | 27 ++++++++++++++++++++ src/ch/meson.build | 2 ++ 4 files changed, 92 insertions(+) create mode 100644 src/ch/ch_alias.c create mode 100644 src/ch/ch_alias.h diff --git a/po/POTFILES b/po/POTFILES index dc7293d0cd..181a36f541 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -19,6 +19,7 @@ src/bhyve/bhyve_firmware.c src/bhyve/bhyve_monitor.c src/bhyve/bhyve_parse_command.c src/bhyve/bhyve_process.c +src/ch/ch_alias.c src/ch/ch_conf.c src/ch/ch_domain.c src/ch/ch_driver.c diff --git a/src/ch/ch_alias.c b/src/ch/ch_alias.c new file mode 100644 index 0000000000..bdf8452a3c --- /dev/null +++ b/src/ch/ch_alias.c @@ -0,0 +1,62 @@ +/* + * ch_alias.c: CH device alias handling + * + * 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 "virutil.h" + +#include "ch_alias.h" + +#define VIR_FROM_THIS VIR_FROM_CH + +int chAssignDeviceDiskAlias(virDomainDiskDef *disk) +{ + const char *prefix = virDomainDiskBusTypeToString(disk->bus); + int idx = -1; + + if (disk->info.alias) { + return 0; + } + + idx = virDiskNameToIndex(disk->dst); + + if (idx < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get index of disk '%1$s'"), + disk->dst); + return -1; + } + + disk->info.alias = g_strdup_printf("%s-disk%d", prefix, idx); + + return 0; +} + +int chAssignDeviceAliases(virDomainDef *def) +{ + size_t i; + + for (i = 0; i < def->ndisks; i++) { + if (chAssignDeviceDiskAlias(def->disks[i]) < 0) + return -1; + } + + /* TODO: handle other devices */ + + return 0; +} diff --git a/src/ch/ch_alias.h b/src/ch/ch_alias.h new file mode 100644 index 0000000000..81e20c27c7 --- /dev/null +++ b/src/ch/ch_alias.h @@ -0,0 +1,27 @@ +/* + * ch_alias.h: CH device alias handling + * + * 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 "domain_conf.h" + +int +chAssignDeviceDiskAlias(virDomainDiskDef *disk); + +int +chAssignDeviceAliases(virDomainDef *def); diff --git a/src/ch/meson.build b/src/ch/meson.build index bba7ee90ee..b3e9c03832 100644 --- a/src/ch/meson.build +++ b/src/ch/meson.build @@ -19,6 +19,8 @@ ch_driver_sources = [ 'ch_hostdev.h', 'ch_hotplug.c', 'ch_hotplug.h', + 'ch_alias.c', + 'ch_alias.h', ] driver_source_files += files(ch_driver_sources) -- 2.50.1
Refactor BuildDiskJson to return a virJSONValue instead of adding the disk json to an json array. This makes the function reusable for hotplugging disks. On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_monitor.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index d369236183..f65cca648b 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -234,43 +234,41 @@ virCHMonitorBuildMemoryJson(virJSONValue *content, virDomainDef *vmdef) return 0; } -static int -virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef) +static virJSONValue* +virCHMonitorBuildDiskJson(virDomainDiskDef *diskdef) { g_autoptr(virJSONValue) disk = virJSONValueNewObject(); if (!diskdef->src) - return -1; + return NULL; switch (diskdef->src->type) { case VIR_STORAGE_TYPE_FILE: if (!diskdef->src->path) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Missing disk file path in domain")); - return -1; + return NULL; } if (!diskdef->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing disk alias")); - return -1; + return NULL; } if (diskdef->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { virReportError(VIR_ERR_INVALID_ARG, _("Only virtio bus types are supported for '%1$s'"), diskdef->src->path); - return -1; + return NULL; } if (virJSONValueObjectAppendString(disk, "path", diskdef->src->path) < 0) - return -1; + return NULL; if (diskdef->src->readonly) { if (virJSONValueObjectAppendBoolean(disk, "readonly", true) < 0) - return -1; + return NULL; } if (virJSONValueObjectAppendString(disk, "id", diskdef->info.alias) < 0) { - return -1; + return NULL; } - if (virJSONValueArrayAppend(disks, &disk) < 0) - return -1; break; case VIR_STORAGE_TYPE_NONE: @@ -284,23 +282,26 @@ virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef) case VIR_STORAGE_TYPE_LAST: default: virReportEnumRangeError(virStorageType, diskdef->src->type); - return -1; + return NULL; } - return 0; + return g_steal_pointer(&disk); } static int virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef) { g_autoptr(virJSONValue) disks = NULL; + g_autoptr(virJSONValue) disk = NULL; size_t i; if (vmdef->ndisks > 0) { disks = virJSONValueNewArray(); for (i = 0; i < vmdef->ndisks; i++) { - if (virCHMonitorBuildDiskJson(disks, vmdef->disks[i]) < 0) + if ((disk = virCHMonitorBuildDiskJson(vmdef->disks[i])) == NULL) + return -1; + if (virJSONValueArrayAppend(disks, &disk) < 0) return -1; } if (virJSONValueObjectAppend(content, "disks", &disks) < 0) -- 2.50.1
On 9/4/25 14:10, Stefan Kober wrote:
Refactor BuildDiskJson to return a virJSONValue instead of adding the disk json to an json array. This makes the function reusable for hotplugging disks.
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_monitor.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index d369236183..f65cca648b 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -234,43 +234,41 @@ virCHMonitorBuildMemoryJson(virJSONValue *content, virDomainDef *vmdef) return 0; }
-static int -virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef) +static virJSONValue* +virCHMonitorBuildDiskJson(virDomainDiskDef *diskdef) { g_autoptr(virJSONValue) disk = virJSONValueNewObject();
if (!diskdef->src) - return -1; + return NULL;
switch (diskdef->src->type) { case VIR_STORAGE_TYPE_FILE: if (!diskdef->src->path) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Missing disk file path in domain")); - return -1; + return NULL; } if (!diskdef->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing disk alias")); - return -1; + return NULL; } if (diskdef->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { virReportError(VIR_ERR_INVALID_ARG, _("Only virtio bus types are supported for '%1$s'"), diskdef->src->path); - return -1; + return NULL; } if (virJSONValueObjectAppendString(disk, "path", diskdef->src->path) < 0) - return -1; + return NULL; if (diskdef->src->readonly) { if (virJSONValueObjectAppendBoolean(disk, "readonly", true) < 0) - return -1; + return NULL; } if (virJSONValueObjectAppendString(disk, "id", diskdef->info.alias) < 0) { - return -1; + return NULL; } - if (virJSONValueArrayAppend(disks, &disk) < 0) - return -1;
break; case VIR_STORAGE_TYPE_NONE: @@ -284,23 +282,26 @@ virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef) case VIR_STORAGE_TYPE_LAST: default: virReportEnumRangeError(virStorageType, diskdef->src->type); - return -1; + return NULL; }
- return 0; + return g_steal_pointer(&disk); }
static int virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef) { g_autoptr(virJSONValue) disks = NULL; + g_autoptr(virJSONValue) disk = NULL;
This variable ...
size_t i;
if (vmdef->ndisks > 0) { disks = virJSONValueNewArray();
for (i = 0; i < vmdef->ndisks; i++) {
... is used exclusively in this loop. Declaring it here not only improves readability (I don't have to hold yet another variable inside my head when reading this function) but also avoids unexpected reuse of the variable. In fact, this whole function could be written a bit better. One level of nesting could be dropped, but that's outside of the scope of this particular patch.
- if (virCHMonitorBuildDiskJson(disks, vmdef->disks[i]) < 0) + if ((disk = virCHMonitorBuildDiskJson(vmdef->disks[i])) == NULL) + return -1; + if (virJSONValueArrayAppend(disks, &disk) < 0) return -1; } if (virJSONValueObjectAppend(content, "disks", &disks) < 0)
Michal
This allows users to call API endpoints that require passing data in a generic way. Previously, only virCHMonitorPutNoContent was offered. On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_monitor.c | 49 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index f65cca648b..2ebeb46ad4 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -62,6 +62,10 @@ VIR_ONCE_GLOBAL_INIT(virCHMonitor); int virCHMonitorShutdownVMM(virCHMonitor *mon); int virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, domainLogContext *logCtxt); +int +virCHMonitorPut(virCHMonitor *mon, const char *endpoint, + virJSONValue *payload, domainLogContext *logCtxt, + virJSONValue** answer); static int virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef) @@ -868,11 +872,15 @@ curl_callback(void *contents, size_t size, size_t nmemb, void *userp) } int -virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, - domainLogContext *logCtxt) +virCHMonitorPut(virCHMonitor *mon, + const char *endpoint, + virJSONValue *payload, + domainLogContext *logCtxt, + virJSONValue **answer) { VIR_LOCK_GUARD lock = virObjectLockGuard(mon); g_autofree char *url = NULL; + g_autofree char *payload_str = NULL; int responseCode = 0; int ret = -1; struct curl_data data = {0}; @@ -890,28 +898,55 @@ virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, curl_easy_setopt(mon->handle, CURLOPT_INFILESIZE, 0L); headers = curl_slist_append(headers, "Accept: application/json"); + curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER, headers); curl_easy_setopt(mon->handle, CURLOPT_WRITEFUNCTION, curl_callback); curl_easy_setopt(mon->handle, CURLOPT_WRITEDATA, (void *)&data); + if (payload) { + payload_str = virJSONValueToString(payload, false); + curl_easy_setopt(mon->handle, CURLOPT_POSTFIELDS, payload_str); + curl_easy_setopt(mon->handle, CURLOPT_CUSTOMREQUEST, "PUT"); + headers = curl_slist_append(headers, "Content-Type: application/json"); + } + responseCode = virCHMonitorCurlPerform(mon->handle); + data.content = g_realloc(data.content, data.size + 1); + data.content[data.size] = 0; + if (logCtxt && data.size) { /* Do this to append a NULL char at the end of data */ - data.content = g_realloc(data.content, data.size + 1); - data.content[data.size] = 0; domainLogContextWrite(logCtxt, "HTTP response code from CH: %d\n", responseCode); domainLogContextWrite(logCtxt, "Response = %s\n", data.content); } - if (responseCode == 200 || responseCode == 204) - ret = 0; + if (responseCode != 200 && responseCode != 204) { + ret = -1; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid HTTP response code from CH: %1$d"), + responseCode); + goto cleanup; + } - curl_slist_free_all(headers); + if (answer) + *answer = virJSONValueFromString(data.content); + + ret = 0; + cleanup: + curl_slist_free_all(headers); + g_free(data.content); return ret; } +int +virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, + domainLogContext *logCtxt) +{ + return virCHMonitorPut(mon, endpoint, NULL, logCtxt, NULL); +} + static int virCHMonitorGet(virCHMonitor *mon, const char *endpoint, virJSONValue **response) { -- 2.50.1
On 9/4/25 14:10, Stefan Kober wrote:
This allows users to call API endpoints that require passing data in a generic way. Previously, only virCHMonitorPutNoContent was offered.
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_monitor.c | 49 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 7 deletions(-)
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index f65cca648b..2ebeb46ad4 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -62,6 +62,10 @@ VIR_ONCE_GLOBAL_INIT(virCHMonitor); int virCHMonitorShutdownVMM(virCHMonitor *mon); int virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, domainLogContext *logCtxt); +int +virCHMonitorPut(virCHMonitor *mon, const char *endpoint, + virJSONValue *payload, domainLogContext *logCtxt, + virJSONValue** answer);
This function should be static. It's not exposed outside of this source file.
static int virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef) @@ -868,11 +872,15 @@ curl_callback(void *contents, size_t size, size_t nmemb, void *userp) }
int -virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, - domainLogContext *logCtxt) +virCHMonitorPut(virCHMonitor *mon, + const char *endpoint, + virJSONValue *payload, + domainLogContext *logCtxt, + virJSONValue **answer) { VIR_LOCK_GUARD lock = virObjectLockGuard(mon); g_autofree char *url = NULL; + g_autofree char *payload_str = NULL; int responseCode = 0; int ret = -1; struct curl_data data = {0}; @@ -890,28 +898,55 @@ virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, curl_easy_setopt(mon->handle, CURLOPT_INFILESIZE, 0L);
headers = curl_slist_append(headers, "Accept: application/json"); + curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER, headers); curl_easy_setopt(mon->handle, CURLOPT_WRITEFUNCTION, curl_callback); curl_easy_setopt(mon->handle, CURLOPT_WRITEDATA, (void *)&data);
+ if (payload) { + payload_str = virJSONValueToString(payload, false); + curl_easy_setopt(mon->handle, CURLOPT_POSTFIELDS, payload_str); + curl_easy_setopt(mon->handle, CURLOPT_CUSTOMREQUEST, "PUT"); + headers = curl_slist_append(headers, "Content-Type: application/json"); + } + responseCode = virCHMonitorCurlPerform(mon->handle);
+ data.content = g_realloc(data.content, data.size + 1); + data.content[data.size] = 0;
Even though I don't know of any architecture, where '\0' byte would be different to zero we somehow prefer the former. Mostly, because it shows we're working with characters.
+ if (logCtxt && data.size) { /* Do this to append a NULL char at the end of data */ - data.content = g_realloc(data.content, data.size + 1); - data.content[data.size] = 0; domainLogContextWrite(logCtxt, "HTTP response code from CH: %d\n", responseCode); domainLogContextWrite(logCtxt, "Response = %s\n", data.content); }
- if (responseCode == 200 || responseCode == 204) - ret = 0; + if (responseCode != 200 && responseCode != 204) { + ret = -1; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid HTTP response code from CH: %1$d"), + responseCode); + goto cleanup; + }
- curl_slist_free_all(headers); + if (answer) + *answer = virJSONValueFromString(data.content); + + ret = 0;
+ cleanup: + curl_slist_free_all(headers); + g_free(data.content); return ret; }
+int +virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, + domainLogContext *logCtxt) +{ + return virCHMonitorPut(mon, endpoint, NULL, logCtxt, NULL); +} + static int virCHMonitorGet(virCHMonitor *mon, const char *endpoint, virJSONValue **response) {
Michal
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_monitor.c | 20 ++++++++++++++++++++ src/ch/ch_monitor.h | 4 ++++ 2 files changed, 24 insertions(+) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 2ebeb46ad4..710ba06d2d 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -315,6 +315,26 @@ virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef) return 0; } +int +virCHMonitorAddDisk(virCHMonitor *monitor, + virDomainDiskDef *diskdef) +{ + g_autoptr(virJSONValue) disk = virCHMonitorBuildDiskJson(diskdef); + g_autoptr(virJSONValue) response = NULL; + + if (!disk) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not build disk json")); + return -1; + } + + return virCHMonitorPut(monitor, + URL_VM_ADD_DISK, + disk, + NULL, + NULL); +} + static int virCHMonitorBuildRngJson(virJSONValue *content, virDomainDef *vmdef) { diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h index ffac9e938e..8338059c7c 100644 --- a/src/ch/ch_monitor.h +++ b/src/ch/ch_monitor.h @@ -40,6 +40,7 @@ #define URL_VM_INFO "vm.info" #define URL_VM_SAVE "vm.snapshot" #define URL_VM_RESTORE "vm.restore" +#define URL_VM_ADD_DISK "vm.add-disk" #define VIRCH_THREAD_NAME_LEN 16 @@ -138,6 +139,9 @@ int virCHMonitorBuildNetJson(virDomainNetDef *netdef, int netindex, char **jsonstr); +int +virCHMonitorAddDisk(virCHMonitor* mon, virDomainDiskDef *diskdef); + int virCHMonitorBuildRestoreJson(virDomainDef *vmdef, const char *from, char **jsonstr); -- 2.50.1
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- po/POTFILES | 1 + src/ch/ch_hotplug.c | 85 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/po/POTFILES b/po/POTFILES index 181a36f541..23da794f84 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -25,6 +25,7 @@ src/ch/ch_domain.c src/ch/ch_driver.c src/ch/ch_events.c src/ch/ch_hostdev.c +src/ch/ch_hotplug.c src/ch/ch_interface.c src/ch/ch_monitor.c src/ch/ch_process.c diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index c46628e7e9..7de688dc44 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -18,10 +18,95 @@ #include <config.h> +#include "ch_alias.h" +#include "ch_domain.h" #include "ch_hotplug.h" +#include "domain_event.h" +#include "domain_validate.h" +#include "virlog.h" + #define VIR_FROM_THIS VIR_FROM_CH +VIR_LOG_INIT("ch.ch_hotplug"); + +static int +chDomainAddDisk(virCHMonitor *mon, virDomainObj *vm, virDomainDiskDef *disk) +{ + if (chAssignDeviceDiskAlias(disk) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Assigning disk alias failed")); + return -1; + } + + if (virCHMonitorAddDisk(mon, disk) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Adding disk to domain failed")); + return -1; + } + + virDomainDiskInsert(vm->def, disk); + + return 0; +} + +G_GNUC_UNUSED +static int +chDomainAttachDeviceLive(virDomainObj *vm, + virDomainDeviceDef *dev) +{ + int ret = -1; + virCHDomainObjPrivate *priv = vm->privateData; + virCHMonitor *mon = priv->monitor; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_DISK: { + if (chDomainAddDisk(mon, vm, dev->data.disk) < 0) { + break; + } + + dev->data.disk = NULL; + ret = 0; + break; + } + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_VSOCK: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_AUDIO: + case VIR_DOMAIN_DEVICE_CRYPTO: + case VIR_DOMAIN_DEVICE_PSTORE: + case VIR_DOMAIN_DEVICE_LAST: + default: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live attach of device '%1$s' is not supported"), + virDomainDeviceTypeToString(dev->type)); + break; + } + + return ret; +} + int chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm G_GNUC_UNUSED, virCHDriver *driver G_GNUC_UNUSED, -- 2.50.1
On 9/4/25 14:10, Stefan Kober wrote:
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- po/POTFILES | 1 + src/ch/ch_hotplug.c | 85 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+)
diff --git a/po/POTFILES b/po/POTFILES index 181a36f541..23da794f84 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -25,6 +25,7 @@ src/ch/ch_domain.c src/ch/ch_driver.c src/ch/ch_events.c src/ch/ch_hostdev.c +src/ch/ch_hotplug.c src/ch/ch_interface.c src/ch/ch_monitor.c src/ch/ch_process.c diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index c46628e7e9..7de688dc44 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -18,10 +18,95 @@
#include <config.h>
+#include "ch_alias.h" +#include "ch_domain.h" #include "ch_hotplug.h"
ch_hotplug.h should be included first as it may already include other files. I mean, you correctly include ch_conf.h inside of ch_hotplug.h because of the virCHDriver type. Again, not something that is critical in this particular case, it's mostly good practice.
+#include "domain_event.h" +#include "domain_validate.h" +#include "virlog.h" + #define VIR_FROM_THIS VIR_FROM_CH
+VIR_LOG_INIT("ch.ch_hotplug"); + +static int +chDomainAddDisk(virCHMonitor *mon, virDomainObj *vm, virDomainDiskDef *disk) +{ + if (chAssignDeviceDiskAlias(disk) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Assigning disk alias failed")); + return -1; + } + + if (virCHMonitorAddDisk(mon, disk) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Adding disk to domain failed")); + return -1; + } + + virDomainDiskInsert(vm->def, disk); + + return 0; +} + +G_GNUC_UNUSED +static int +chDomainAttachDeviceLive(virDomainObj *vm, + virDomainDeviceDef *dev) +{ + int ret = -1; + virCHDomainObjPrivate *priv = vm->privateData; + virCHMonitor *mon = priv->monitor; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_DISK: { + if (chDomainAddDisk(mon, vm, dev->data.disk) < 0) { + break; + } + + dev->data.disk = NULL; + ret = 0; + break; + } + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_VSOCK: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_AUDIO: + case VIR_DOMAIN_DEVICE_CRYPTO: + case VIR_DOMAIN_DEVICE_PSTORE: + case VIR_DOMAIN_DEVICE_LAST: + default: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live attach of device '%1$s' is not supported"), + virDomainDeviceTypeToString(dev->type)); + break; + } + + return ret; +} + int chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm G_GNUC_UNUSED, virCHDriver *driver G_GNUC_UNUSED,
This is great place to implement this function ^^^. IOW, just move corresponding change from the next patch here. Then you don't need to annotate a function as unused. Michal
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_driver.c | 44 +++++++++++++++++++++++++++++++++++++++++ src/ch/ch_hotplug.c | 48 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index cf6874f22e..4f4783efb1 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -25,6 +25,7 @@ #include "ch_conf.h" #include "ch_domain.h" #include "ch_driver.h" +#include "ch_hotplug.h" #include "ch_monitor.h" #include "ch_process.h" #include "domain_cgroup.h" @@ -2344,6 +2345,47 @@ chDomainInterfaceAddresses(virDomain *dom, return ret; } +static int +chDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + virCHDriver *driver = dom->conn->privateData; + virDomainObj *vm = NULL; + int ret = -1; + + if (!(vm = virCHDomainObjFromDomain(dom))) + goto cleanup; + + if (virDomainAttachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; + + if (chDomainAttachDeviceLiveAndUpdateConfig(vm, driver, xml, flags) < 0) { + goto endjob; + } + + ret = 0; + + endjob: + virDomainObjEndJob(vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + +static int +chDomainAttachDevice(virDomainPtr dom, + const char *xml) +{ + return chDomainAttachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE); +} /* Function Tables */ static virHypervisorDriver chHypervisorDriver = { @@ -2406,6 +2448,8 @@ static virHypervisorDriver chHypervisorDriver = { .connectDomainEventRegisterAny = chConnectDomainEventRegisterAny, /* 10.10.0 */ .connectDomainEventDeregisterAny = chConnectDomainEventDeregisterAny, /* 10.10.0 */ .domainInterfaceAddresses = chDomainInterfaceAddresses, /* 11.0.0 */ + .domainAttachDevice = chDomainAttachDevice, /* 11.8.0 */ + .domainAttachDeviceFlags = chDomainAttachDeviceFlags, /* 11.8.0 */ }; static virConnectDriver chConnectDriver = { diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index 7de688dc44..524355b93c 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -50,7 +50,6 @@ chDomainAddDisk(virCHMonitor *mon, virDomainObj *vm, virDomainDiskDef *disk) return 0; } -G_GNUC_UNUSED static int chDomainAttachDeviceLive(virDomainObj *vm, virDomainDeviceDef *dev) @@ -108,13 +107,52 @@ chDomainAttachDeviceLive(virDomainObj *vm, } int -chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm G_GNUC_UNUSED, - virCHDriver *driver G_GNUC_UNUSED, - const char *xml G_GNUC_UNUSED, +chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm, + virCHDriver *driver, + const char *xml, unsigned int flags) { + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; + g_autoptr(virDomainDeviceDef) devLive = NULL; + g_autoptr(virDomainDef) vmdef = NULL; + g_autoptr(virCHDriverConfig) cfg = NULL; + g_autoptr(virDomainDeviceDef) devConf = NULL; + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - return -1; + cfg = virCHDriverGetConfig(driver); + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Persistent domain state changes are not supported")); + return -1; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!(devLive = virDomainDeviceDefParse(xml, vm->def, + driver->xmlopt, NULL, + parse_flags))) { + return -1; + } + + if (virDomainDeviceValidateAliasForHotplug(vm, devLive, + VIR_DOMAIN_AFFECT_LIVE) < 0) + return -1; + + if (virDomainDefCompatibleDevice(vm->def, devLive, NULL, + VIR_DOMAIN_DEVICE_ACTION_ATTACH, + true) < 0) { + return -1; + } + + if (chDomainAttachDeviceLive(vm, devLive) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to add device")); + return -1; + } + } + + return 0; } -- 2.50.1
On 9/4/25 14:10, Stefan Kober wrote:
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_driver.c | 44 +++++++++++++++++++++++++++++++++++++++++ src/ch/ch_hotplug.c | 48 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 87 insertions(+), 5 deletions(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index cf6874f22e..4f4783efb1 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -25,6 +25,7 @@ #include "ch_conf.h" #include "ch_domain.h" #include "ch_driver.h" +#include "ch_hotplug.h" #include "ch_monitor.h" #include "ch_process.h" #include "domain_cgroup.h" @@ -2344,6 +2345,47 @@ chDomainInterfaceAddresses(virDomain *dom, return ret; }
+static int +chDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + virCHDriver *driver = dom->conn->privateData; + virDomainObj *vm = NULL; + int ret = -1; + + if (!(vm = virCHDomainObjFromDomain(dom))) + goto cleanup; + + if (virDomainAttachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; + + if (chDomainAttachDeviceLiveAndUpdateConfig(vm, driver, xml, flags) < 0) { + goto endjob; + } + + ret = 0; + + endjob: + virDomainObjEndJob(vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + +static int +chDomainAttachDevice(virDomainPtr dom, + const char *xml) +{ + return chDomainAttachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE); +}
/* Function Tables */ static virHypervisorDriver chHypervisorDriver = { @@ -2406,6 +2448,8 @@ static virHypervisorDriver chHypervisorDriver = { .connectDomainEventRegisterAny = chConnectDomainEventRegisterAny, /* 10.10.0 */ .connectDomainEventDeregisterAny = chConnectDomainEventDeregisterAny, /* 10.10.0 */ .domainInterfaceAddresses = chDomainInterfaceAddresses, /* 11.0.0 */ + .domainAttachDevice = chDomainAttachDevice, /* 11.8.0 */ + .domainAttachDeviceFlags = chDomainAttachDeviceFlags, /* 11.8.0 */ };
static virConnectDriver chConnectDriver = {
Until here the change makes sense. But what's below should be part of previous patch.
diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index 7de688dc44..524355b93c 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -50,7 +50,6 @@ chDomainAddDisk(virCHMonitor *mon, virDomainObj *vm, virDomainDiskDef *disk) return 0; }
-G_GNUC_UNUSED static int chDomainAttachDeviceLive(virDomainObj *vm, virDomainDeviceDef *dev) @@ -108,13 +107,52 @@ chDomainAttachDeviceLive(virDomainObj *vm, }
int -chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm G_GNUC_UNUSED, - virCHDriver *driver G_GNUC_UNUSED, - const char *xml G_GNUC_UNUSED, +chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm, + virCHDriver *driver, + const char *xml, unsigned int flags) { + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; + g_autoptr(virDomainDeviceDef) devLive = NULL; + g_autoptr(virDomainDef) vmdef = NULL; + g_autoptr(virCHDriverConfig) cfg = NULL; + g_autoptr(virDomainDeviceDef) devConf = NULL; + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);
- return -1; + cfg = virCHDriverGetConfig(driver); + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Persistent domain state changes are not supported"));
At first I was a bit puzzled why have this at all. I mean, virCheckFlags() could have just contained _AFFECT_LIVE but then it hit me: this produces much more descriptive error message. So let's keep it then.
+ return -1; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!(devLive = virDomainDeviceDefParse(xml, vm->def, + driver->xmlopt, NULL, + parse_flags))) { + return -1; + } + + if (virDomainDeviceValidateAliasForHotplug(vm, devLive, + VIR_DOMAIN_AFFECT_LIVE) < 0) + return -1; + + if (virDomainDefCompatibleDevice(vm->def, devLive, NULL, + VIR_DOMAIN_DEVICE_ACTION_ATTACH, + true) < 0) { + return -1; + } + + if (chDomainAttachDeviceLive(vm, devLive) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to add device")); + return -1; + } + } + + return 0; }
Michal
This is required to have unique device aliases for devices throughout the domain lifecycle. On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_process.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index cd2e88af1e..550bfefae1 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -24,6 +24,7 @@ #include <fcntl.h> #include <poll.h> +#include "ch_alias.h" #include "ch_domain.h" #include "ch_monitor.h" #include "ch_process.h" @@ -889,6 +890,9 @@ virCHProcessPrepareDomain(virDomainObj *vm) if (virCHProcessPrepareDomainHostdevs(vm) < 0) return -1; + if (chAssignDeviceAliases(vm->def) < 0) + return -1; + return 0; } -- 2.50.1
On 9/4/25 14:10, Stefan Kober wrote:
This is required to have unique device aliases for devices throughout the domain lifecycle.
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_process.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index cd2e88af1e..550bfefae1 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -24,6 +24,7 @@ #include <fcntl.h> #include <poll.h>
+#include "ch_alias.h" #include "ch_domain.h" #include "ch_monitor.h" #include "ch_process.h" @@ -889,6 +890,9 @@ virCHProcessPrepareDomain(virDomainObj *vm) if (virCHProcessPrepareDomainHostdevs(vm) < 0) return -1;
+ if (chAssignDeviceAliases(vm->def) < 0) + return -1; +
I'd do this before preparing hostdevs. While it's strictly not needed currently, it may happen that a hostdev code would expect device aliases to be set (e.g. in case of QEMU driver the SCSI detach code expects that).
return 0; }
Michal
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_monitor.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 710ba06d2d..6ed78c09c7 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -584,15 +584,25 @@ virCHMonitorBuildVMJson(virCHDriver *driver, virDomainDef *vmdef, return 0; } +static virJSONValue* +virCHMonitorBuildKeyValueJson(const char *key, + const char *value) +{ + g_autoptr(virJSONValue) content = virJSONValueNewObject(); + + if (virJSONValueObjectAppendString(content, key, value) < 0) + return NULL; + + return g_steal_pointer(&content); +} + static int virCHMonitorBuildKeyValueStringJson(char **jsonstr, const char *key, const char *value) { - g_autoptr(virJSONValue) content = virJSONValueNewObject(); - - if (virJSONValueObjectAppendString(content, key, value) < 0) - return -1; + g_autoptr(virJSONValue) content = + virCHMonitorBuildKeyValueJson(key, value); if (!(*jsonstr = virJSONValueToString(content, false))) return -1; -- 2.50.1
On 9/4/25 14:10, Stefan Kober wrote:
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_monitor.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 710ba06d2d..6ed78c09c7 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -584,15 +584,25 @@ virCHMonitorBuildVMJson(virCHDriver *driver, virDomainDef *vmdef, return 0; }
+static virJSONValue* +virCHMonitorBuildKeyValueJson(const char *key, + const char *value) +{ + g_autoptr(virJSONValue) content = virJSONValueNewObject(); + + if (virJSONValueObjectAppendString(content, key, value) < 0) + return NULL; + + return g_steal_pointer(&content); +} + static int virCHMonitorBuildKeyValueStringJson(char **jsonstr, const char *key, const char *value) { - g_autoptr(virJSONValue) content = virJSONValueNewObject(); - - if (virJSONValueObjectAppendString(content, key, value) < 0) - return -1; + g_autoptr(virJSONValue) content = + virCHMonitorBuildKeyValueJson(key, value);
This fits onto one line perfectly.
if (!(*jsonstr = virJSONValueToString(content, false))) return -1;
Michal
The function calls the respective CH API to remove a device of any type from a VM. On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_monitor.c | 11 +++++++++++ src/ch/ch_monitor.h | 3 +++ 2 files changed, 14 insertions(+) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 6ed78c09c7..d75d696019 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -610,6 +610,17 @@ virCHMonitorBuildKeyValueStringJson(char **jsonstr, return 0; } +int virCHMonitorRemoveDevice(virCHMonitor *mon, + const char* device_id) +{ + g_autoptr(virJSONValue) payload = + virCHMonitorBuildKeyValueJson("id", device_id); + + VIR_DEBUG("Remove device %s", device_id); + + return virCHMonitorPut(mon, URL_VM_REMOVE_DEVICE, payload, NULL, NULL); +} + static int chMonitorCreateSocket(const char *socket_path) { diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h index 8338059c7c..e8fa393e54 100644 --- a/src/ch/ch_monitor.h +++ b/src/ch/ch_monitor.h @@ -41,6 +41,7 @@ #define URL_VM_SAVE "vm.snapshot" #define URL_VM_RESTORE "vm.restore" #define URL_VM_ADD_DISK "vm.add-disk" +#define URL_VM_REMOVE_DEVICE "vm.remove-device" #define VIRCH_THREAD_NAME_LEN 16 @@ -141,6 +142,8 @@ virCHMonitorBuildNetJson(virDomainNetDef *netdef, char **jsonstr); int virCHMonitorAddDisk(virCHMonitor* mon, virDomainDiskDef *diskdef); +int virCHMonitorRemoveDevice(virCHMonitor *mon, + const char* device_id); int virCHMonitorBuildRestoreJson(virDomainDef *vmdef, const char *from, -- 2.50.1
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_driver.c | 42 +++++++++++ src/ch/ch_hotplug.c | 175 ++++++++++++++++++++++++++++++++++++++++++++ src/ch/ch_hotplug.h | 6 ++ 3 files changed, 223 insertions(+) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 4f4783efb1..760fccba82 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -2387,6 +2387,46 @@ chDomainAttachDevice(virDomainPtr dom, return chDomainAttachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE); } +static int +chDomainDetachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + virCHDriver *driver = dom->conn->privateData; + virDomainObj *vm = NULL; + int ret = -1; + + if (!(vm = virCHDomainObjFromDomain(dom))) + goto cleanup; + + if (virDomainDetachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; + + if (chDomainDetachDeviceLiveAndUpdateConfig(driver, vm, xml, flags) < 0) + goto endjob; + + ret = 0; + + endjob: + virDomainObjEndJob(vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + +static int chDomainDetachDevice(virDomainPtr dom, const char *xml) +{ + return chDomainDetachDeviceFlags(dom, xml, + VIR_DOMAIN_AFFECT_LIVE); +} + /* Function Tables */ static virHypervisorDriver chHypervisorDriver = { .name = "CH", @@ -2450,6 +2490,8 @@ static virHypervisorDriver chHypervisorDriver = { .domainInterfaceAddresses = chDomainInterfaceAddresses, /* 11.0.0 */ .domainAttachDevice = chDomainAttachDevice, /* 11.8.0 */ .domainAttachDeviceFlags = chDomainAttachDeviceFlags, /* 11.8.0 */ + .domainDetachDevice = chDomainDetachDevice, /* 11.8.0 */ + .domainDetachDeviceFlags = chDomainDetachDeviceFlags, /* 11.8.0 */ }; static virConnectDriver chConnectDriver = { diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index 524355b93c..95fe1f0f6f 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -156,3 +156,178 @@ chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm, return 0; } + +static int +chFindDiskId(virDomainDef *def, const char *dst) +{ + size_t i; + + for (i = 0; i < def->ndisks; i++) { + if (STREQ(def->disks[i]->dst, dst)) + return i; + } + + return -1; +} + + +/** + * chDomainFindDisk + * + * Helper function to find a disk device definition of a domain. + * + * Searches through the disk devices of a domain by comparing to 'match' and + * returns any match via the 'detach' out parameter. + */ +static int +chDomainFindDisk(virDomainObj *vm, + virDomainDiskDef *match, + virDomainDiskDef **detach) +{ + virDomainDiskDef *disk; + int idx; + + if ((idx = chFindDiskId(vm->def, match->dst)) < 0) { + virReportError(VIR_ERR_DEVICE_MISSING, + _("disk %1$s not found"), match->dst); + return -1; + } + *detach = disk = vm->def->disks[idx]; + + return 0; +} + +static int +chDomainDetachDeviceLive(virDomainObj *vm, + virDomainDeviceDef *match) +{ + virDomainDeviceDef detach = { .type = match->type }; + virDomainDeviceInfo *info = NULL; + virCHDomainObjPrivate *priv = vm->privateData; + int idx = 0; + + switch (match->type) { + case VIR_DOMAIN_DEVICE_DISK: + if (chDomainFindDisk(vm, match->data.disk, + &detach.data.disk) < 0) { + return -1; + } + break; + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_VSOCK: + case VIR_DOMAIN_DEVICE_AUDIO: + case VIR_DOMAIN_DEVICE_CRYPTO: + case VIR_DOMAIN_DEVICE_PSTORE: + case VIR_DOMAIN_DEVICE_LAST: + case VIR_DOMAIN_DEVICE_NONE: + default: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live detach of device '%1$s' is not supported"), + virDomainDeviceTypeToString(match->type)); + return -1; + } + + /* "detach" now points to the actual device we want to detach */ + + if (!(info = virDomainDeviceGetInfo(&detach))) { + /* + * This should never happen, since all of the device types in + * the switch cases that end with a "break" instead of a + * return have a virDeviceInfo in them. + */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("device of type '%1$s' has no device info"), + virDomainDeviceTypeToString(detach.type)); + return -1; + } + + /* Make generic validation checks common to all device types */ + + if (!info->alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot detach %1$s device with no alias"), + virDomainDeviceTypeToString(detach.type)); + return -1; + } + + if (virCHMonitorRemoveDevice(priv->monitor, info->alias) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid response from CH. Disk removal failed.")); + return -1; + } + + if (match->type == VIR_DOMAIN_DEVICE_DISK) { + idx = chFindDiskId(vm->def, match->data.disk->dst); + if (idx >= 0) { + virDomainDiskRemove(vm->def, idx); + } + } + + return 0; +} + +int +chDomainDetachDeviceLiveAndUpdateConfig(virCHDriver *driver, + virDomainObj *vm, + const char *xml, + unsigned int flags) +{ + g_autoptr(virCHDriverConfig) cfg = NULL; + g_autoptr(virDomainDeviceDef) dev_config = NULL; + g_autoptr(virDomainDeviceDef) dev_live = NULL; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + g_autoptr(virDomainDef) vmdef = NULL; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + cfg = virCHDriverGetConfig(driver); + + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && + !(flags & VIR_DOMAIN_AFFECT_LIVE)) + parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE; + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Persistent domain state changes are not supported")); + return -1; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!(dev_live = virDomainDeviceDefParse(xml, vm->def, driver->xmlopt, + NULL, parse_flags))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not parse domain definition")); + return -1; + } + + if (chDomainDetachDeviceLive(vm, dev_live) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could detach device")); + return -1; + } + } + + return 0; +} diff --git a/src/ch/ch_hotplug.h b/src/ch/ch_hotplug.h index 04915ba5de..4a9b9b3b3e 100644 --- a/src/ch/ch_hotplug.h +++ b/src/ch/ch_hotplug.h @@ -25,3 +25,9 @@ chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm, virCHDriver *driver, const char *xml, unsigned int flags); + +int +chDomainDetachDeviceLiveAndUpdateConfig(virCHDriver *driver, + virDomainObj *vm, + const char *xml, + unsigned int flags); -- 2.50.1
On 9/4/25 14:10, Stefan Kober wrote:
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_driver.c | 42 +++++++++++ src/ch/ch_hotplug.c | 175 ++++++++++++++++++++++++++++++++++++++++++++ src/ch/ch_hotplug.h | 6 ++ 3 files changed, 223 insertions(+)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 4f4783efb1..760fccba82 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -2387,6 +2387,46 @@ chDomainAttachDevice(virDomainPtr dom, return chDomainAttachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE); }
+static int +chDomainDetachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + virCHDriver *driver = dom->conn->privateData; + virDomainObj *vm = NULL; + int ret = -1; + + if (!(vm = virCHDomainObjFromDomain(dom))) + goto cleanup; + + if (virDomainDetachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; + + if (chDomainDetachDeviceLiveAndUpdateConfig(driver, vm, xml, flags) < 0) + goto endjob; + + ret = 0; + + endjob: + virDomainObjEndJob(vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + +static int chDomainDetachDevice(virDomainPtr dom, const char *xml) +{ + return chDomainDetachDeviceFlags(dom, xml, + VIR_DOMAIN_AFFECT_LIVE); +} + /* Function Tables */ static virHypervisorDriver chHypervisorDriver = { .name = "CH", @@ -2450,6 +2490,8 @@ static virHypervisorDriver chHypervisorDriver = { .domainInterfaceAddresses = chDomainInterfaceAddresses, /* 11.0.0 */ .domainAttachDevice = chDomainAttachDevice, /* 11.8.0 */ .domainAttachDeviceFlags = chDomainAttachDeviceFlags, /* 11.8.0 */ + .domainDetachDevice = chDomainDetachDevice, /* 11.8.0 */ + .domainDetachDeviceFlags = chDomainDetachDeviceFlags, /* 11.8.0 */ };
static virConnectDriver chConnectDriver = {
Again, until here the patch is fine and what's below should be moved into a separate patch.
diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index 524355b93c..95fe1f0f6f 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -156,3 +156,178 @@ chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm,
return 0; } + +static int +chFindDiskId(virDomainDef *def, const char *dst) +{ + size_t i; + + for (i = 0; i < def->ndisks; i++) { + if (STREQ(def->disks[i]->dst, dst)) + return i; + } + + return -1; +} + + +/** + * chDomainFindDisk + * + * Helper function to find a disk device definition of a domain. + * + * Searches through the disk devices of a domain by comparing to 'match' and + * returns any match via the 'detach' out parameter. + */ +static int +chDomainFindDisk(virDomainObj *vm, + virDomainDiskDef *match, + virDomainDiskDef **detach) +{ + virDomainDiskDef *disk; + int idx; + + if ((idx = chFindDiskId(vm->def, match->dst)) < 0) { + virReportError(VIR_ERR_DEVICE_MISSING, + _("disk %1$s not found"), match->dst); + return -1; + } + *detach = disk = vm->def->disks[idx]; + + return 0; +} + +static int +chDomainDetachDeviceLive(virDomainObj *vm, + virDomainDeviceDef *match) +{ + virDomainDeviceDef detach = { .type = match->type }; + virDomainDeviceInfo *info = NULL; + virCHDomainObjPrivate *priv = vm->privateData; + int idx = 0; + + switch (match->type) { + case VIR_DOMAIN_DEVICE_DISK: + if (chDomainFindDisk(vm, match->data.disk, + &detach.data.disk) < 0) { + return -1; + } + break; + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_VSOCK: + case VIR_DOMAIN_DEVICE_AUDIO: + case VIR_DOMAIN_DEVICE_CRYPTO: + case VIR_DOMAIN_DEVICE_PSTORE: + case VIR_DOMAIN_DEVICE_LAST: + case VIR_DOMAIN_DEVICE_NONE: + default: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live detach of device '%1$s' is not supported"), + virDomainDeviceTypeToString(match->type)); + return -1; + } + + /* "detach" now points to the actual device we want to detach */ + + if (!(info = virDomainDeviceGetInfo(&detach))) { + /* + * This should never happen, since all of the device types in + * the switch cases that end with a "break" instead of a + * return have a virDeviceInfo in them. + */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("device of type '%1$s' has no device info"), + virDomainDeviceTypeToString(detach.type)); + return -1; + } + + /* Make generic validation checks common to all device types */ + + if (!info->alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot detach %1$s device with no alias"), + virDomainDeviceTypeToString(detach.type)); + return -1; + } + + if (virCHMonitorRemoveDevice(priv->monitor, info->alias) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid response from CH. Disk removal failed.")); + return -1; + } + + if (match->type == VIR_DOMAIN_DEVICE_DISK) { + idx = chFindDiskId(vm->def, match->data.disk->dst); + if (idx >= 0) { + virDomainDiskRemove(vm->def, idx); + } + }
Now, this little if() bothered me for a while. I think we need some function, like chDomainDeviceRemove() with proper switch() over all device types.
+ + return 0; +} + +int +chDomainDetachDeviceLiveAndUpdateConfig(virCHDriver *driver, + virDomainObj *vm, + const char *xml, + unsigned int flags) +{ + g_autoptr(virCHDriverConfig) cfg = NULL; + g_autoptr(virDomainDeviceDef) dev_config = NULL; + g_autoptr(virDomainDeviceDef) dev_live = NULL; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + g_autoptr(virDomainDef) vmdef = NULL; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + cfg = virCHDriverGetConfig(driver); + + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && + !(flags & VIR_DOMAIN_AFFECT_LIVE)) + parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE; + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Persistent domain state changes are not supported")); + return -1; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!(dev_live = virDomainDeviceDefParse(xml, vm->def, driver->xmlopt, + NULL, parse_flags))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not parse domain definition")); + return -1; + } + + if (chDomainDetachDeviceLive(vm, dev_live) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could detach device")); + return -1; + } + } + + return 0; +} diff --git a/src/ch/ch_hotplug.h b/src/ch/ch_hotplug.h index 04915ba5de..4a9b9b3b3e 100644 --- a/src/ch/ch_hotplug.h +++ b/src/ch/ch_hotplug.h @@ -25,3 +25,9 @@ chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm, virCHDriver *driver, const char *xml, unsigned int flags); + +int +chDomainDetachDeviceLiveAndUpdateConfig(virCHDriver *driver, + virDomainObj *vm, + const char *xml, + unsigned int flags);
Michal
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- NEWS.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 9577be0213..9b64489954 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,11 @@ v11.8.0 (unreleased) * **New features** + * ch: Disk hotplug Support + + Users can now attach and detach disks of Cloud Hypervisor domains at + runtime. + * **Improvements** * **Bug fixes** -- 2.50.1
On 9/4/25 14:10, Stefan Kober wrote:
This patchset adds the necessary functionality to support disk hotplugging in the CH driver.
Some alias handling that can be extended to other device types is added, in order to allow detaching the devices via the Cloud Hypervisor API.
Stefan Kober (13): ch: add ch_hotplug.{h,c} files to CH build ch: pass disk alias to CHV ch: add ch_alias.{c,h} for device alias handling ch: refactor virCHMonitorBuildDiskJson ch: add/use virCHMonitorPut function ch: add monitor disk attach logic ch: add disk attach helper functions ch: implement disk attach in public API ch: assign aliases in ProcessPrepareDomain ch: add virCHMonitorBuildKeyValueJson ch: add virCHMonitorRemoveDevice function ch: implement disk device detach in public API NEWS: announce disk hotplug support for ch
NEWS.rst | 5 + po/POTFILES | 2 + src/ch/ch_alias.c | 62 +++++++++ src/ch/ch_alias.h | 27 ++++ src/ch/ch_driver.c | 86 ++++++++++++ src/ch/ch_hotplug.c | 333 ++++++++++++++++++++++++++++++++++++++++++++ src/ch/ch_hotplug.h | 33 +++++ src/ch/ch_monitor.c | 131 ++++++++++++++--- src/ch/ch_monitor.h | 7 + src/ch/ch_process.c | 4 + src/ch/meson.build | 4 + 11 files changed, 671 insertions(+), 23 deletions(-) create mode 100644 src/ch/ch_alias.c create mode 100644 src/ch/ch_alias.h create mode 100644 src/ch/ch_hotplug.c create mode 100644 src/ch/ch_hotplug.h
This looks way better than previous version. Thank you for working in my suggestions. There are still some small problems, but I'll fix them before merging. Also, I'm moving some hunks between some patches AND I'm reordering some patches too. For instance, patch 02/13 introduces a temporary regression as it requires aliases to be assigned to disks, but that doesn't happen until patch 09/13. If these are reordered then no regression occurs. Anyway, good job! What should be done in a follow up series is to emit events on device attach/detach. Management apps will certainly value that. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and merged. Michal
Thanks for merging and taking care of the finishing touches! And thanks for still leaving comments about how to improve our style next time right away. As a next step, we will be preparing patches that introduce handling for persistent domains.
On 9/9/25 09:25, stefan.kober@cyberus-technology.de wrote:
Thanks for merging and taking care of the finishing touches!
Glad to help. We have some specifics when it comes to structuring patches and code style. Nobody gets it right the first time :-D
And thanks for still leaving comments about how to improve our style next time right away.
Yeah, I figured that making you send another version would be discouraging so I rather fixed your patches and merged them.
As a next step, we will be preparing patches that introduce handling for persistent domains.
That sounds good. Michal
participants (3)
-
Michal Prívozník -
Stefan Kober -
stefan.kober＠cyberus-technology.de