[PATCH 0/5] ch: Misc cleanups
*** BLURB HERE *** Michal Prívozník (5): ch_process: Avoid memleak in chProcessAddNetworkDevice() ch: Use correct domain definition in chDomainGetXMLDesc() ch: Set transient domain definition ch: Assign device alias early ch: Sort driver sources and drop header files src/ch/ch_driver.c | 9 ++++++++- src/ch/ch_hotplug.c | 23 ++++++++++++++++++++--- src/ch/ch_process.c | 13 +++++++++---- src/ch/meson.build | 17 +++-------------- 4 files changed, 40 insertions(+), 22 deletions(-) -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> The 'payload' variable inside of chProcessAddNetworkDevice() is reused and thus the memory it points to just before its repurpose is not freed. Avoid reusing g_autofree variables. 128 bytes in 1 blocks are definitely lost in loss record 1,828 of 2,026 at 0x491A120: realloc (vg_replace_malloc.c:1801) by 0x4FEC251: g_realloc (in /usr/lib64/libglib-2.0.so.0.8400.4) by 0x500BB7E: g_string_expand (in /usr/lib64/libglib-2.0.so.0.8400.4) by 0x500BBF0: g_string_sized_new (in /usr/lib64/libglib-2.0.so.0.8400.4) by 0x4A114C0: virBufferInitialize (virbuffer.c:121) by 0x4A11890: virBufferAdd (virbuffer.c:160) by 0x4A67344: virJSONValueToBuffer (virjson.c:1562) by 0x4A673DB: virJSONValueToString (virjson.c:1599) by 0xBC878AB: virCHMonitorBuildNetJson (ch_monitor.c:466) by 0xBC8D4A9: chProcessAddNetworkDevice (ch_process.c:688) by 0xBC8FCE2: chDomainAttachDeviceLive (ch_hotplug.c:78) by 0xBC900CA: chDomainAttachDeviceLiveAndUpdateConfig (ch_hotplug.c:174) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_process.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index a1f30f09e1..4ebb261805 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -637,6 +637,7 @@ chProcessAddNetworkDevice(virCHDriver *driver, g_auto(virBuffer) http_headers = VIR_BUFFER_INITIALIZER; g_autofree int *tapfds = NULL; g_autofree char *payload = NULL; + g_autofree char *netJSONPayload = NULL; g_autofree char *response = NULL; size_t tapfd_len; size_t payload_len; @@ -685,15 +686,15 @@ chProcessAddNetworkDevice(virCHDriver *driver, } chAssignDeviceNetAlias(vmdef, net); - if (virCHMonitorBuildNetJson(net, &payload) < 0) { + if (virCHMonitorBuildNetJson(net, &netJSONPayload) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to build net json")); return -1; } virBufferAsprintf(&buf, "%s", virBufferCurrentContent(&http_headers)); - virBufferAsprintf(&buf, "Content-Length: %zu\r\n\r\n", strlen(payload)); - virBufferAsprintf(&buf, "%s", payload); + virBufferAsprintf(&buf, "Content-Length: %zu\r\n\r\n", strlen(netJSONPayload)); + virBufferAddStr(&buf, netJSONPayload); payload_len = virBufferUse(&buf); payload = virBufferContentAndReset(&buf); -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> The chDomainGetXMLDesc() function claims to support VIR_DOMAIN_XML_INACTIVE to obtain the persistent definition of a running domain (in its call to virCheckFlags()) but in fact, it's always passing vm->def to virDomainDefFormat(). So far, there's no harm done because CH driver never sets domain def as transient. But that'll change. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_driver.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index ad13306c4c..8ec90e1192 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -1277,6 +1277,7 @@ static char *chDomainGetXMLDesc(virDomainPtr dom, { virCHDriver *driver = dom->conn->privateData; virDomainObj *vm; + virDomainDef *def; char *ret = NULL; virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); @@ -1287,7 +1288,13 @@ static char *chDomainGetXMLDesc(virDomainPtr dom, if (virDomainGetXMLDescEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - ret = virDomainDefFormat(vm->def, driver->xmlopt, + if ((flags & VIR_DOMAIN_XML_INACTIVE) && vm->newDef) { + def = vm->newDef; + } else { + def = vm->def; + } + + ret = virDomainDefFormat(def, driver->xmlopt, virDomainDefFormatConvertXMLFlags(flags)); cleanup: -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> Libvirt's philosophy is that for a running domain there are two (in general distinct) definitions: live definition (reflects the running state) and inactive definition (used to seed the live definition when domain is being created). That's why we have VIR_DOMAIN_AFFECT_LIVE and VIR_DOMAIN_AFFECT_CONFIG flags to APIs that modify domain definitions. Well, the CH driver doesn't do this distinction. Fix this by making the domain definition transient when it's being created. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_process.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 4ebb261805..f16f2b3916 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -951,6 +951,10 @@ virCHProcessStart(virCHDriver *driver, return -1; } + VIR_DEBUG("Setting current domain def as transient"); + if (virDomainObjSetDefTransient(driver->xmlopt, vm, NULL) < 0) + return -1; + VIR_DEBUG("Creating domain log file for %s domain", vm->def->name); if (!(logCtxt = domainLogContextNew(cfg->stdioLogD, cfg->logDir, CH_DRIVER_NAME, @@ -1100,6 +1104,7 @@ virCHProcessStop(virCHDriver *driver, virHostdevReAttachDomainDevices(driver->hostdevMgr, CH_DRIVER_NAME, def, hostdev_flags); + virDomainObjRemoveTransientDef(vm); virErrorRestore(&orig_err); return 0; } -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> Assigning device should happen from ch_hotplug.c (just like it's done for disks currently) not in ch_process.c. Move alias assignment out of chProcessAddNetworkDevice(). And while at it, mimic what's done with disks and have net hotplug handling done from a function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_hotplug.c | 23 ++++++++++++++++++++--- src/ch/ch_process.c | 1 - 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index fabca2a73a..b296fd11b1 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -53,6 +53,25 @@ chDomainAddDisk(virCHMonitor *mon, return 0; } + +static int +chDomainAddNet(virCHDriver *driver, + virCHMonitor *mon, + virDomainObj *vm, + virDomainNetDef *net) +{ + chAssignDeviceNetAlias(vm->def, net); + + if (chProcessAddNetworkDevice(driver, mon, vm->def, net, NULL, NULL) < 0) { + return -1; + } + + virDomainNetInsert(vm->def, net); + + return 0; +} + + static int chDomainAttachDeviceLive(virCHDriver *driver, virDomainObj *vm, @@ -75,12 +94,10 @@ chDomainAttachDeviceLive(virCHDriver *driver, break; case VIR_DOMAIN_DEVICE_NET: - if (chProcessAddNetworkDevice(driver, mon, vm->def, dev->data.net, - NULL, NULL) < 0) { + if (chDomainAddNet(driver, mon, vm, dev->data.net) < 0) { break; } - virDomainNetInsert(vm->def, dev->data.net); alias = dev->data.net->info.alias; dev->data.net = NULL; ret = 0; diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index f16f2b3916..29db853a7f 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -685,7 +685,6 @@ chProcessAddNetworkDevice(virCHDriver *driver, return -1; } - chAssignDeviceNetAlias(vmdef, net); if (virCHMonitorBuildNetJson(net, &netJSONPayload) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to build net json")); -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> Firstly, there's no need to list header files in ch_driver_sources (we don't do that anywhere else, and meson is smart enough to figure them out). And secondly, the list of source file is not sorted which means new source files are added in random order. Thus, drop header files from the list and sort it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/meson.build | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/src/ch/meson.build b/src/ch/meson.build index b3e9c03832..aef2d86533 100644 --- a/src/ch/meson.build +++ b/src/ch/meson.build @@ -1,26 +1,15 @@ ch_driver_sources = [ - 'ch_capabilities.h', + 'ch_alias.c', 'ch_capabilities.c', 'ch_conf.c', - 'ch_conf.h', 'ch_domain.c', - 'ch_domain.h', 'ch_driver.c', - 'ch_driver.h', 'ch_events.c', - 'ch_events.h', + 'ch_hostdev.c', + 'ch_hotplug.c', 'ch_interface.c', - 'ch_interface.h', 'ch_monitor.c', - 'ch_monitor.h', 'ch_process.c', - 'ch_process.h', - 'ch_hostdev.c', - 'ch_hostdev.h', - 'ch_hotplug.c', - 'ch_hotplug.h', - 'ch_alias.c', - 'ch_alias.h', ] driver_source_files += files(ch_driver_sources) -- 2.51.0
On Fri, Nov 07, 2025 at 12:24:54 +0100, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (5): ch_process: Avoid memleak in chProcessAddNetworkDevice() ch: Use correct domain definition in chDomainGetXMLDesc() ch: Set transient domain definition ch: Assign device alias early ch: Sort driver sources and drop header files
src/ch/ch_driver.c | 9 ++++++++- src/ch/ch_hotplug.c | 23 ++++++++++++++++++++--- src/ch/ch_process.c | 13 +++++++++---- src/ch/meson.build | 17 +++-------------- 4 files changed, 40 insertions(+), 22 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
participants (2)
-
Jiri Denemark -
Michal Privoznik