[PATCH 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 DomainAttachDevice skeletons ch: add DomainDetachDevice skeletons ch: add ch_hotplug.{h,c} files to CH build ch: call into ch_hotplug implementation ch: pass disk alias to CHV ch: add ch_alias.{c,h} for device alias handling ch: add virCHMonitorPut function ch: use MonitorPut in MonitorPutNoContent ch: add disk attach functionality ch: assign aliases in ProcessPrepareDomain ch: Add virCHMonitorRemoveDevice function ch: implement disk device hot removal NEWS: announce disk hotplug support for ch NEWS.rst | 5 + po/POTFILES | 1 + src/ch/ch_alias.c | 59 ++++++++ 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 | 81 +++++++++-- src/ch/ch_monitor.h | 7 + src/ch/ch_process.c | 4 + src/ch/meson.build | 4 + 11 files changed, 630 insertions(+), 10 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
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_driver.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index cf6874f22e..30cfb22649 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -2344,6 +2344,40 @@ chDomainInterfaceAddresses(virDomain *dom, return ret; } +static int +chDomainAttachDeviceFlags(virDomainPtr dom, + const char */*xml*/, + unsigned int flags) +{ + 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; + + 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 +2440,9 @@ static virHypervisorDriver chHypervisorDriver = { .connectDomainEventRegisterAny = chConnectDomainEventRegisterAny, /* 10.10.0 */ .connectDomainEventDeregisterAny = chConnectDomainEventDeregisterAny, /* 10.10.0 */ .domainInterfaceAddresses = chDomainInterfaceAddresses, /* 11.0.0 */ + .domainAttachDevice = chDomainAttachDevice, /* 11.7.0 */ + .domainAttachDeviceFlags = chDomainAttachDeviceFlags, /* 11.7.0 */ + }; static virConnectDriver chConnectDriver = { -- 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 | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 30cfb22649..c64b2ba511 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -2379,6 +2379,40 @@ chDomainAttachDevice(virDomainPtr dom, return chDomainAttachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE); } +static int +chDomainDetachDeviceFlags(virDomainPtr dom, + const char */*xml*/, + unsigned int flags) +{ + 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; + + 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", @@ -2442,7 +2476,8 @@ static virHypervisorDriver chHypervisorDriver = { .domainInterfaceAddresses = chDomainInterfaceAddresses, /* 11.0.0 */ .domainAttachDevice = chDomainAttachDevice, /* 11.7.0 */ .domainAttachDeviceFlags = chDomainAttachDeviceFlags, /* 11.7.0 */ - + .domainDetachDevice = chDomainDetachDevice, /* 11.7.0 */ + .domainDetachDeviceFlags = chDomainDetachDeviceFlags, /* 11.7.0 */ }; static virConnectDriver chConnectDriver = { -- 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> --- po/POTFILES | 1 + src/ch/ch_hotplug.c | 35 +++++++++++++++++++++++++++++++++++ src/ch/ch_hotplug.h | 27 +++++++++++++++++++++++++++ src/ch/meson.build | 2 ++ 4 files changed, 65 insertions(+) create mode 100644 src/ch/ch_hotplug.c create mode 100644 src/ch/ch_hotplug.h diff --git a/po/POTFILES b/po/POTFILES index dc7293d0cd..50a055fb73 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -24,6 +24,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 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-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_driver.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index c64b2ba511..39f9d934c0 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" @@ -2346,9 +2347,10 @@ chDomainInterfaceAddresses(virDomain *dom, static int chDomainAttachDeviceFlags(virDomainPtr dom, - const char */*xml*/, + const char *xml, unsigned int flags) { + virCHDriver *driver = dom->conn->privateData; virDomainObj *vm = NULL; int ret = -1; @@ -2364,6 +2366,12 @@ chDomainAttachDeviceFlags(virDomainPtr dom, if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) goto endjob; + if (chDomainAttachDeviceLiveAndConfig(vm, driver, xml, flags) < 0) { + goto endjob; + } + + ret = 0; + endjob: virDomainObjEndJob(vm); -- 2.50.1
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> --- src/ch/ch_alias.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++ src/ch/ch_alias.h | 27 +++++++++++++++++++++ src/ch/meson.build | 2 ++ 3 files changed, 88 insertions(+) create mode 100644 src/ch/ch_alias.c create mode 100644 src/ch/ch_alias.h diff --git a/src/ch/ch_alias.c b/src/ch/ch_alias.c new file mode 100644 index 0000000000..63bcd9f212 --- /dev/null +++ b/src/ch/ch_alias.c @@ -0,0 +1,59 @@ +/* + * 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) { + 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
On 8/28/25 14:54, Stefan Kober wrote:
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_alias.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++ src/ch/ch_alias.h | 27 +++++++++++++++++++++ src/ch/meson.build | 2 ++ 3 files changed, 88 insertions(+) create mode 100644 src/ch/ch_alias.c create mode 100644 src/ch/ch_alias.h
diff --git a/src/ch/ch_alias.c b/src/ch/ch_alias.c new file mode 100644 index 0000000000..63bcd9f212 --- /dev/null +++ b/src/ch/ch_alias.c @@ -0,0 +1,59 @@ +/* + * 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) { + return -1;
virDiskNameToIndex() doesn't report any error on failure. If one occurs this propagates the error with no error message. Might be worth reporting an error message here.
+ } + + 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; +}
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 | 61 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index d369236183..63c8425b4b 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, + const char *payload, domainLogContext *logCtxt, + virJSONValue** answer); static int virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef) @@ -866,6 +870,63 @@ curl_callback(void *contents, size_t size, size_t nmemb, void *userp) return content_size; } +int +virCHMonitorPut(virCHMonitor *mon, const char *endpoint, + const char *payload, domainLogContext *logCtxt, + virJSONValue** answer) +{ + VIR_LOCK_GUARD lock = virObjectLockGuard(mon); + g_autofree char *url = NULL; + int responseCode = 0; + struct curl_data data = {0}; + struct curl_slist *headers = NULL; + + url = g_strdup_printf("%s/%s", URL_ROOT, endpoint); + + /* reset all options of a libcurl session handle at first */ + curl_easy_reset(mon->handle); + + curl_easy_setopt(mon->handle, CURLOPT_UNIX_SOCKET_PATH, mon->socketpath); + curl_easy_setopt(mon->handle, CURLOPT_URL, url); + curl_easy_setopt(mon->handle, CURLOPT_UPLOAD, 1L); + curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER, NULL); + curl_easy_setopt(mon->handle, CURLOPT_INFILESIZE, 0L); + + headers = curl_slist_append(headers, "Content-Type: 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) { + curl_easy_setopt(mon->handle, CURLOPT_POSTFIELDS, payload); + curl_easy_setopt(mon->handle, CURLOPT_CUSTOMREQUEST, "PUT"); + headers = curl_slist_append(headers, "Accept: 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 */ + domainLogContextWrite(logCtxt, "HTTP response code from CH: %d\n", responseCode); + domainLogContextWrite(logCtxt, "Response = %s\n", data.content); + } + + curl_slist_free_all(headers); + + if (responseCode != 200 && responseCode != 204) { + return -1; + } + + if (answer) + *answer = virJSONValueFromString(data.content); + + return 0; +} + int virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, domainLogContext *logCtxt) -- 2.50.1
On 8/28/25 14:54, 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 | 61 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index d369236183..63c8425b4b 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, + const char *payload, domainLogContext *logCtxt, + virJSONValue** answer);
One argument per line please. Oh, and we like pointers to be declared as: virJSONValue **answer instead of virJSONValue** answer
static int virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef) @@ -866,6 +870,63 @@ curl_callback(void *contents, size_t size, size_t nmemb, void *userp) return content_size; }
+int +virCHMonitorPut(virCHMonitor *mon, const char *endpoint, + const char *payload, domainLogContext *logCtxt, + virJSONValue** answer) +{ + VIR_LOCK_GUARD lock = virObjectLockGuard(mon); + g_autofree char *url = NULL; + int responseCode = 0; + struct curl_data data = {0}; + struct curl_slist *headers = NULL; + + url = g_strdup_printf("%s/%s", URL_ROOT, endpoint); + + /* reset all options of a libcurl session handle at first */ + curl_easy_reset(mon->handle); + + curl_easy_setopt(mon->handle, CURLOPT_UNIX_SOCKET_PATH, mon->socketpath); + curl_easy_setopt(mon->handle, CURLOPT_URL, url); + curl_easy_setopt(mon->handle, CURLOPT_UPLOAD, 1L); + curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER, NULL); + curl_easy_setopt(mon->handle, CURLOPT_INFILESIZE, 0L); + + headers = curl_slist_append(headers, "Content-Type: application/json");
I believe here you want "Accept: application/json" put inside 'headers', because if 'payload' is NULL, we are not sending anything, thus 'Content-Type' header field makes no sense.
+ + 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) { + curl_easy_setopt(mon->handle, CURLOPT_POSTFIELDS, payload); + curl_easy_setopt(mon->handle, CURLOPT_CUSTOMREQUEST, "PUT"); + headers = curl_slist_append(headers, "Accept: application/json");
And here you want the 'Content-type: application/json' header field. Now, this got me thinking, since we set here 'application/json' type regardless of the actual 'payload' data type, would it make sense to turn 'payload' from 'const char *' to 'virJSONValue *'? This branch would then call virJSONValueToString().
+ } + + 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 */ + domainLogContextWrite(logCtxt, "HTTP response code from CH: %d\n", responseCode); + domainLogContextWrite(logCtxt, "Response = %s\n", data.content); + } + + curl_slist_free_all(headers); + + if (responseCode != 200 && responseCode != 204) { + return -1; + } + + if (answer) + *answer = virJSONValueFromString(data.content);
I think data.content needs to be freed after this, no matter what. Otherwise it'll leak.
+ + return 0; +} + int virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, domainLogContext *logCtxt)
Michal
Very good catches! I have seen there are builds enabling the address sanitizers. Is there any good workflow to find leaks in a structured way? Can I enable address sanitizer in a libvirt build and just run some tests on that to see if anything leaks or are there things preventing that?
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_monitor.c | 40 +--------------------------------------- 1 file changed, 1 insertion(+), 39 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 63c8425b4b..5f3e2adbee 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -931,45 +931,7 @@ int virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, domainLogContext *logCtxt) { - VIR_LOCK_GUARD lock = virObjectLockGuard(mon); - g_autofree char *url = NULL; - int responseCode = 0; - int ret = -1; - struct curl_data data = {0}; - struct curl_slist *headers = NULL; - - url = g_strdup_printf("%s/%s", URL_ROOT, endpoint); - - /* reset all options of a libcurl session handle at first */ - curl_easy_reset(mon->handle); - - curl_easy_setopt(mon->handle, CURLOPT_UNIX_SOCKET_PATH, mon->socketpath); - curl_easy_setopt(mon->handle, CURLOPT_URL, url); - curl_easy_setopt(mon->handle, CURLOPT_UPLOAD, 1L); - curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER, NULL); - 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); - - responseCode = virCHMonitorCurlPerform(mon->handle); - - 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; - - curl_slist_free_all(headers); - - return ret; + return virCHMonitorPut(mon, endpoint, NULL, logCtxt, NULL); } static int -- 2.50.1
On 8/28/25 14:54, 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 | 40 +--------------------------------------- 1 file changed, 1 insertion(+), 39 deletions(-)
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 63c8425b4b..5f3e2adbee 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -931,45 +931,7 @@ int virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, domainLogContext *logCtxt) { - VIR_LOCK_GUARD lock = virObjectLockGuard(mon); - g_autofree char *url = NULL; - int responseCode = 0; - int ret = -1; - struct curl_data data = {0}; - struct curl_slist *headers = NULL; - - url = g_strdup_printf("%s/%s", URL_ROOT, endpoint); - - /* reset all options of a libcurl session handle at first */ - curl_easy_reset(mon->handle); - - curl_easy_setopt(mon->handle, CURLOPT_UNIX_SOCKET_PATH, mon->socketpath); - curl_easy_setopt(mon->handle, CURLOPT_URL, url); - curl_easy_setopt(mon->handle, CURLOPT_UPLOAD, 1L); - curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER, NULL); - 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); - - responseCode = virCHMonitorCurlPerform(mon->handle); - - 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; - - curl_slist_free_all(headers); - - return ret; + return virCHMonitorPut(mon, endpoint, NULL, logCtxt, NULL); }
static int
Maybe it's just a matter of preference, but I'll squash this into the previous one. Michal
On-behalf-of: SAP stefan.kober@sap.com Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de> --- src/ch/ch_driver.c | 2 +- src/ch/ch_hotplug.c | 131 ++++++++++++++++++++++++++++++++++++++++++-- src/ch/ch_monitor.c | 17 ++++++ src/ch/ch_monitor.h | 4 ++ 4 files changed, 149 insertions(+), 5 deletions(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 39f9d934c0..0484201c88 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -2366,7 +2366,7 @@ chDomainAttachDeviceFlags(virDomainPtr dom, if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) goto endjob; - if (chDomainAttachDeviceLiveAndConfig(vm, driver, xml, flags) < 0) { + if (chDomainAttachDeviceLiveAndUpdateConfig(vm, driver, xml, flags) < 0) { goto endjob; } diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index c46628e7e9..524355b93c 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -18,18 +18,141 @@ #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; +} + +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, - 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; } diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 5f3e2adbee..8968d84a71 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -314,6 +314,23 @@ virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef) return 0; } +int +virCHMonitorAddDisk(virCHMonitor *monitor, + virDomainDiskDef *diskdef) +{ + g_autofree char *payload = NULL; + g_autoptr(virJSONValue) disks = virJSONValueNewArray(); + g_autoptr(virJSONValue) response = NULL; + + if (virCHMonitorBuildDiskJson(disks, diskdef) < 0) { + return -1; + } + + payload = virJSONValueToString(virJSONValueArrayGet(disks, 0), false); + + return virCHMonitorPut(monitor, URL_VM_ADD_DISK, payload, 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 8/28/25 14:54, 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 | 2 +- src/ch/ch_hotplug.c | 131 ++++++++++++++++++++++++++++++++++++++++++-- src/ch/ch_monitor.c | 17 ++++++ src/ch/ch_monitor.h | 4 ++ 4 files changed, 149 insertions(+), 5 deletions(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 39f9d934c0..0484201c88 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -2366,7 +2366,7 @@ chDomainAttachDeviceFlags(virDomainPtr dom, if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) goto endjob;
- if (chDomainAttachDeviceLiveAndConfig(vm, driver, xml, flags) < 0) { + if (chDomainAttachDeviceLiveAndUpdateConfig(vm, driver, xml, flags) < 0) {
This should have been squashed to one of previous patches that introduced the incorrect function call.
goto endjob; }
diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index c46628e7e9..524355b93c 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -18,18 +18,141 @@
#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; +} + +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, - 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; }
I'd break this patch into two. The first one adds ch_monitor and other helper code, the other then implements the public API (rendering the patch that added empty skeleton useless).
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 5f3e2adbee..8968d84a71 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -314,6 +314,23 @@ virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef) return 0; }
+int +virCHMonitorAddDisk(virCHMonitor *monitor, + virDomainDiskDef *diskdef) +{ + g_autofree char *payload = NULL; + g_autoptr(virJSONValue) disks = virJSONValueNewArray(); + g_autoptr(virJSONValue) response = NULL; + + if (virCHMonitorBuildDiskJson(disks, diskdef) < 0) {
I wonder whether it would make sense to make virCHMonitorBuildDiskJson() return a virJSONValue of just one disk instead of appending it into an array. Appending could be then moved into the for() loop inside of virCHMonitorBuildDisksJson(). This way we could avoid creating an array here.
+ return -1; + } + + payload = virJSONValueToString(virJSONValueArrayGet(disks, 0), false); + + return virCHMonitorPut(monitor, URL_VM_ADD_DISK, payload, 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);
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
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 | 17 +++++++++++++++++ src/ch/ch_monitor.h | 3 +++ 2 files changed, 20 insertions(+) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 8968d84a71..2e21823f08 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -596,6 +596,23 @@ virCHMonitorBuildKeyValueStringJson(char **jsonstr, return 0; } +int virCHMonitorRemoveDevice(virCHMonitor *mon, + const char* device_id) +{ + g_autofree char *payload = NULL; + + if (virCHMonitorBuildKeyValueStringJson(&payload, "id", device_id) != 0) + return -1; + + VIR_DEBUG("Remove device %s", device_id); + + if (virCHMonitorPut(mon, URL_VM_REMOVE_DEVICE, payload, NULL) == NULL) { + return -1; + } + + return 0; +} + 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 | 8 +- src/ch/ch_hotplug.c | 175 ++++++++++++++++++++++++++++++++++++++++++++ src/ch/ch_hotplug.h | 6 ++ src/ch/ch_monitor.c | 8 +- 4 files changed, 190 insertions(+), 7 deletions(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 0484201c88..c0e42171ae 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -2389,9 +2389,10 @@ chDomainAttachDevice(virDomainPtr dom, static int chDomainDetachDeviceFlags(virDomainPtr dom, - const char */*xml*/, + const char *xml, unsigned int flags) { + virCHDriver *driver = dom->conn->privateData; virDomainObj *vm = NULL; int ret = -1; @@ -2407,6 +2408,11 @@ chDomainDetachDeviceFlags(virDomainPtr dom, if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) goto endjob; + if (chDomainDetachDeviceLiveAndUpdateConfig(driver, vm, xml, flags) < 0) + goto endjob; + + ret = 0; + endjob: virDomainObjEndJob(vm); 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); diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 2e21823f08..2da71e962b 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -604,13 +604,9 @@ int virCHMonitorRemoveDevice(virCHMonitor *mon, if (virCHMonitorBuildKeyValueStringJson(&payload, "id", device_id) != 0) return -1; - VIR_DEBUG("Remove device %s", device_id); + VIR_DEBUG("Remove device with id: %s", device_id); - if (virCHMonitorPut(mon, URL_VM_REMOVE_DEVICE, payload, NULL) == NULL) { - return -1; - } - - return 0; + return virCHMonitorPut(mon, URL_VM_REMOVE_DEVICE, payload, NULL, NULL); } static int -- 2.50.1
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 06cbd8d6c5..185d5a283f 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -32,6 +32,11 @@ v11.7.0 (unreleased) The bhyve driver now supports querying domain block, interface, and memory statistics. Not all statistics fields are supported though. + * ch: Disk hotplug Support + + Users can now attach and detach disks of Cloud Hypervisor domains at + runtime. + * **Improvements** * bhyve: improve 'efi' configuration autofill -- 2.50.1
On Thu, Aug 28, 2025 at 14:54:40 +0200, 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 DomainAttachDevice skeletons ch: add DomainDetachDevice skeletons ch: add ch_hotplug.{h,c} files to CH build ch: call into ch_hotplug implementation ch: pass disk alias to CHV ch: add ch_alias.{c,h} for device alias handling ch: add virCHMonitorPut function ch: use MonitorPut in MonitorPutNoContent ch: add disk attach functionality ch: assign aliases in ProcessPrepareDomain ch: Add virCHMonitorRemoveDevice function ch: implement disk device hot removal NEWS: announce disk hotplug support for ch
I didn't have time (or expertise on the cloud hypervisor specifics) to do proper review but I've noticed that the patches introduce temporary problems (e.g. failed build) in between the patches. Our guidelines state that both the build and tests must pass after each commit (to e.g. allow good bisectability): https://libvirt.org/hacking.html#preparing-patches Make sure that you run a build and test after each commit. No need to send another series yet, but be prepared to fix those problems. Some things my testing showed: ../../../libvirt/src/ch/ch_driver.c:2349:46: error: omitting the parameter name in a function definition is a C23 extension [-Werror,-Wc23-extensions] 2349 | const char */*xml*/, | ^ 1 error generated. - addition to po/POTFILES being misplaced - misplaced function declarations in header files (broken build where function is not declared yet)
On 8/28/25 14:54, 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 DomainAttachDevice skeletons ch: add DomainDetachDevice skeletons ch: add ch_hotplug.{h,c} files to CH build ch: call into ch_hotplug implementation ch: pass disk alias to CHV ch: add ch_alias.{c,h} for device alias handling ch: add virCHMonitorPut function ch: use MonitorPut in MonitorPutNoContent ch: add disk attach functionality ch: assign aliases in ProcessPrepareDomain ch: Add virCHMonitorRemoveDevice function ch: implement disk device hot removal NEWS: announce disk hotplug support for ch
NEWS.rst | 5 + po/POTFILES | 1 + src/ch/ch_alias.c | 59 ++++++++ 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 | 81 +++++++++-- src/ch/ch_monitor.h | 7 + src/ch/ch_process.c | 4 + src/ch/meson.build | 4 + 11 files changed, 630 insertions(+), 10 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
I've taken closer look at these and in general they look good. I mean, what Peter wrote still applies and needs to be addressed in v2, but we're on a track here. Michal
participants (4)
-
Michal Prívozník -
Peter Krempa -
Stefan Kober -
stefan.kober＠cyberus-technology.de