[PATCH 0/3] lxc: Misc cleanups
While reviewing Radoslaw's patched [1] I've noticed some opportunities for improvement. 1: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/2HH7A... Michal Prívozník (3): lxc: Drop pointless g_free() from virLXCProcessStart() lxc: Don't leak @veth in lxcDomainAttachDeviceNetLive() lxc: Rework cleanup section in lxcDomainAttachDeviceNetLive() src/lxc/lxc_driver.c | 30 ++++++++++++++---------------- src/lxc/lxc_process.c | 1 - 2 files changed, 14 insertions(+), 17 deletions(-) -- 2.53.0
From: Michal Privoznik <mprivozn@redhat.com> When staring an LXC domain (well, container) its consoles are opened and each one is assigned an alias (for later use with virDomainOpenConsole()). Now, before generating new alias the old one is freed. But the old one can never be anything other than NULL. The domain is inactive at this point (we are in process of starting it, after all). And LXC driver does not support user aliases, yet. Just drop the pointless g_free(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_process.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 2c0bcb9dd3..cac49af6e2 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1346,7 +1346,6 @@ int virLXCProcessStart(virLXCDriver * driver, g_free(vm->def->consoles[i]->source->data.file.path); vm->def->consoles[i]->source->data.file.path = ttyPath; - g_free(vm->def->consoles[i]->info.alias); vm->def->consoles[i]->info.alias = g_strdup_printf("console%zu", i); } -- 2.53.0
From: Michal Privoznik <mprivozn@redhat.com> During hotplug of an <interface/> into an LXC domain (lxcDomainAttachDeviceNetLive()), the host side name of the interface is stored in @veth variable. Well, all possible paths that set the variable (virLXCProcessSetupInterfaceTap(), virLXCProcessSetupInterfaceDirect()) document it is caller's responsibility to free the memory. But it never does so. ==49848== 12 bytes in 2 blocks are definitely lost in loss record 68 of 1,763 ==49848== at 0x4913888: malloc (vg_replace_malloc.c:447) ==49848== by 0x546F0BC: __vasprintf_internal (in /usr/lib64/libc.so.6) ==49848== by 0x5077A70: g_vasprintf (in /usr/lib64/libglib-2.0.so.0.8400.4) ==49848== by 0x50404DB: g_strdup_vprintf (in /usr/lib64/libglib-2.0.so.0.8400.4) ==49848== by 0x50405A4: g_strdup_printf (in /usr/lib64/libglib-2.0.so.0.8400.4) ==49848== by 0x4A8591E: virNetDevGenerateName (virnetdev.c:3573) ==49848== by 0x4A93C38: virNetDevVethCreate (virnetdevveth.c:124) ==49848== by 0xED6C505: virLXCProcessSetupInterfaceTap (lxc_process.c:279) ==49848== by 0xED5F7A7: lxcDomainAttachDeviceNetLive (lxc_driver.c:3517) ==49848== by 0xED60D24: lxcDomainAttachDeviceLive (lxc_driver.c:3925) ==49848== by 0xED6262D: lxcDomainAttachDeviceFlags (lxc_driver.c:4453) ==49848== by 0xED62819: lxcDomainAttachDevice (lxc_driver.c:4485) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 0b0592a4b7..a1a0343a5b 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3473,7 +3473,7 @@ lxcDomainAttachDeviceNetLive(virLXCDriver *driver, int ret = -1; virDomainNetType actualType; const virNetDevBandwidth *actualBandwidth; - char *veth = NULL; + g_autofree char *veth = NULL; if (!priv->initpid) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", -- 2.53.0
From: Michal Privoznik <mprivozn@redhat.com> The cleanup section in lxcDomainAttachDeviceNetLive() is suspicious. It checks @ret for success and adds net into domain definition. This is not something fits into cleanup. It belongs right before 'ret = 0' line when we know everything before succeeded. Moving that piece of code where it belongs, the cleanup section becomes error because it is executed only in case of failure. Change the label to error, fix corresponding goto-s, and drop @ret variable. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_driver.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index a1a0343a5b..b59c080da9 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3470,7 +3470,6 @@ lxcDomainAttachDeviceNetLive(virLXCDriver *driver, virDomainNetDef *net) { virLXCDomainObjPrivate *priv = vm->privateData; - int ret = -1; virDomainNetType actualType; const virNetDevBandwidth *actualBandwidth; g_autofree char *veth = NULL; @@ -3512,18 +3511,18 @@ lxcDomainAttachDeviceNetLive(virLXCDriver *driver, if (!brname) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No bridge name specified")); - goto cleanup; + return -1; } if (!(veth = virLXCProcessSetupInterfaceTap(vm->def, net, brname))) - goto cleanup; + return -1; } break; case VIR_DOMAIN_NET_TYPE_ETHERNET: if (!(veth = virLXCProcessSetupInterfaceTap(vm->def, net, NULL))) - goto cleanup; + return -1; break; case VIR_DOMAIN_NET_TYPE_DIRECT: { if (!(veth = virLXCProcessSetupInterfaceDirect(driver, vm->def, net))) - goto cleanup; + return -1; } break; case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: @@ -3538,11 +3537,11 @@ lxcDomainAttachDeviceNetLive(virLXCDriver *driver, case VIR_DOMAIN_NET_TYPE_VDS: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Network device type is not supported")); - goto cleanup; + return -1; case VIR_DOMAIN_NET_TYPE_LAST: default: virReportEnumRangeError(virDomainNetType, actualType); - goto cleanup; + return -1; } /* Set bandwidth or warn if requested and not supported. */ actualBandwidth = virDomainNetGetActualBandwidth(net); @@ -3554,7 +3553,7 @@ lxcDomainAttachDeviceNetLive(virLXCDriver *driver, flags |= VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED; if (virNetDevBandwidthSet(net->ifname, actualBandwidth, flags) < 0) - goto cleanup; + goto error; } else { VIR_WARN("setting bandwidth on interfaces of type '%s' is not implemented yet: %s", virDomainNetTypeToString(actualType), virGetLastErrorMessage()); @@ -3563,17 +3562,16 @@ lxcDomainAttachDeviceNetLive(virLXCDriver *driver, if (virNetDevSetNamespace(veth, priv->initpid) < 0) { virDomainAuditNet(vm, NULL, net, "attach", false); - goto cleanup; + goto error; } virDomainAuditNet(vm, NULL, net, "attach", true); - ret = 0; + vm->def->nets[vm->def->nnets++] = net; + return 0; - cleanup: - if (!ret) { - vm->def->nets[vm->def->nnets++] = net; - } else if (veth) { + error: + if (veth) { switch (actualType) { case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: @@ -3603,7 +3601,7 @@ lxcDomainAttachDeviceNetLive(virLXCDriver *driver, } } - return ret; + return -1; } -- 2.53.0
On Thu, Jun 18, 2026 at 10:04:45 +0200, Michal Privoznik via Devel wrote:
While reviewing Radoslaw's patched [1] I've noticed some opportunities for improvement.
1: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/2HH7A...
Michal Prívozník (3): lxc: Drop pointless g_free() from virLXCProcessStart() lxc: Don't leak @veth in lxcDomainAttachDeviceNetLive() lxc: Rework cleanup section in lxcDomainAttachDeviceNetLive()
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Michal Privoznik -
Peter Krempa