[PATCH 0/4] ch: Disk attach/detach fixes
While reviewing Stefan patches [1] I've changed some of them before merging and oopsie, I've introduced a bug (patch 1/4) but fortunately forgot to remove the code I intended to move so it actually works :-D. 1: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HAZRW... Michal Prívozník (4): ch: Actually remove device in chDomainDetachDeviceLive() ch: Drop deadcode from chDomainDetachDeviceLive() ch: Drop useless variable in chDomainFindDisk() ch: Avoid memleak on disk detach in chDomainRemoveDevice() src/ch/ch_hotplug.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) -- 2.49.1
From: Michal Privoznik <mprivozn@redhat.com> Inside of chDomainDetachDeviceLive() there are two variables that are important in this case: 'match' and 'detach'. The first one contains device definition as parsed from user provided XML, the other contains pointer to the device definition inside virDomainDef (as returned by chDomainFindDisk()). Now, when chDomainRemoveDevice() is called, it looks up the device inside virDomainDef and removes it (using pointer comparison). Well, that means 'detach' must be passed as an argument instead of 'match'. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_hotplug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index 058954035d..b06fdbe5a8 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -336,7 +336,7 @@ chDomainDetachDeviceLive(virDomainObj *vm, return -1; } - if (chDomainRemoveDevice(vm, match) < 0) + if (chDomainRemoveDevice(vm, &detach) < 0) return -1; if (match->type == VIR_DOMAIN_DEVICE_DISK) { -- 2.49.1
From: Michal Privoznik <mprivozn@redhat.com> At the end of chDomainDetachDeviceLive() there's a code that tries to remove the disk that's being detached from the domain definition. Well, it's a leftover from the original patch which I forgot to remove when rewriting it to use chDomainRemoveDevice(). The disk is removed there so this code has no chance in removing it again. Drop the code. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_hotplug.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index b06fdbe5a8..aa723dd123 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -263,7 +263,6 @@ chDomainDetachDeviceLive(virDomainObj *vm, virDomainDeviceDef detach = { .type = match->type }; virDomainDeviceInfo *info = NULL; virCHDomainObjPrivate *priv = vm->privateData; - int idx = 0; switch (match->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -339,13 +338,6 @@ chDomainDetachDeviceLive(virDomainObj *vm, if (chDomainRemoveDevice(vm, &detach) < 0) 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; } -- 2.49.1
From: Michal Privoznik <mprivozn@redhat.com> The 'disk' variable inside of chDomainFindDisk() is not used really. Drop it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_hotplug.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index aa723dd123..cfa11cc5e5 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -185,7 +185,6 @@ chDomainFindDisk(virDomainObj *vm, virDomainDiskDef *match, virDomainDiskDef **detach) { - virDomainDiskDef *disk; int idx; if ((idx = chFindDiskId(vm->def, match->dst)) < 0) { @@ -193,7 +192,7 @@ chDomainFindDisk(virDomainObj *vm, _("disk %1$s not found"), match->dst); return -1; } - *detach = disk = vm->def->disks[idx]; + *detach = vm->def->disks[idx]; return 0; } -- 2.49.1
From: Michal Privoznik <mprivozn@redhat.com> The aim of chDomainRemoveDevice() is to remove device from virDomainDef. Well, in case of disks this is done by calling virDomainDiskRemove() which merely just removes it from the array of virDomainDiskDef-s but leaves it up to the caller to actually free the disk def. 1,286 (560 direct, 726 indirect) bytes in 1 blocks are definitely lost in loss record 2,005 of 2,041 at 0x4919EF3: calloc (vg_replace_malloc.c:1675) by 0x4FEB249: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8400.3) by 0x4AFD9A4: virDomainDiskDefNewSource (domain_conf.c:2409) by 0x4B10ACA: virDomainDiskDefParseXML (domain_conf.c:8509) by 0x4B24F07: virDomainDeviceDefParse (domain_conf.c:14631) by 0xB8D8881: chDomainAttachDeviceLiveAndUpdateConfig (ch_hotplug.c:135) by 0xB8CCFE0: chDomainAttachDeviceFlags (ch_driver.c:2376) by 0xB8CD057: chDomainAttachDevice (ch_driver.c:2394) by 0x4DC1C7D: virDomainAttachDevice (libvirt-domain.c:8951) by 0x405E545: remoteDispatchDomainAttachDevice (remote_daemon_dispatch_stubs.h:3763) by 0x405E495: remoteDispatchDomainAttachDeviceHelper (remote_daemon_dispatch_stubs.h:3742) by 0x4BF3164: virNetServerProgramDispatchCall (virnetserverprogram.c:423) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_hotplug.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c index cfa11cc5e5..0a55a57069 100644 --- a/src/ch/ch_hotplug.c +++ b/src/ch/ch_hotplug.c @@ -212,6 +212,7 @@ chDomainRemoveDevice(virDomainObj *vm, for (i = 0; i < vm->def->ndisks; i++) { if (vm->def->disks[i] == device->data.disk) { virDomainDiskRemove(vm->def, i); + g_clear_pointer(&device->data.disk, virDomainDiskDefFree); break; } } -- 2.49.1
On a Wednesday in 2025, Michal Privoznik via Devel wrote:
While reviewing Stefan patches [1] I've changed some of them before merging and oopsie, I've introduced a bug (patch 1/4) but fortunately forgot to remove the code I intended to move so it actually works :-D.
1: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HAZRW...
Michal Prívozník (4): ch: Actually remove device in chDomainDetachDeviceLive() ch: Drop deadcode from chDomainDetachDeviceLive() ch: Drop useless variable in chDomainFindDisk() ch: Avoid memleak on disk detach in chDomainRemoveDevice()
src/ch/ch_hotplug.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik