[PATCH 0/7] conf: qemu: Use automatic memory management
Refactored the code to use GLib automatic memory management capabilities. Added macros to enable the g_autoptr capability for some structures. Removed prevosly defined cleanup stages, as now it is not required. Moved some variables to closer scope, to enbale auto memory management capability for some variables. Kirill Shchetiniuk (7): conf: Use automatic memory management qemu: Use automatic memory management virQEMUCapsKVMSupportsNesting: Use automatic memory management qemuDomainSetVcpusAgent: Use automatic memory management qemuDomainGetHostnameLease: Use automatic memory management qemuProcessRefreshChannelVirtioState: Use automatic memory management qemuMigrationSrcPerformPeer2Peer2: Use automatic memory management src/conf/domain_conf.c | 35 +++++++++++++------------------- src/conf/domain_conf.h | 2 ++ src/datatypes.h | 1 + src/qemu/qemu_capabilities.c | 6 +++--- src/qemu/qemu_domain.c | 15 ++++++-------- src/qemu/qemu_driver.c | 33 +++++++++++------------------- src/qemu/qemu_migration.c | 39 +++++++++++++----------------------- src/qemu/qemu_monitor.c | 6 ++---- src/qemu/qemu_process.c | 7 +++---- 9 files changed, 57 insertions(+), 87 deletions(-) -- 2.49.0
From: Kirill Shchetiniuk <kshcheti@redhat.com> Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/conf/domain_conf.c | 35 ++++++++++++++--------------------- src/conf/domain_conf.h | 2 ++ 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 396cd1c0db..27e286ff52 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3433,18 +3433,17 @@ void virDomainShmemDefFree(virDomainShmemDef *def) virDomainVideoDef * virDomainVideoDefNew(virDomainXMLOption *xmlopt) { - virDomainVideoDef *def; + g_autoptr(virDomainVideoDef) def = NULL; def = g_new0(virDomainVideoDef, 1); if (xmlopt && xmlopt->privateData.videoNew && !(def->privateData = xmlopt->privateData.videoNew())) { - VIR_FREE(def); return NULL; } def->heads = 1; - return def; + return g_steal_pointer(&def); } @@ -3496,17 +3495,16 @@ virDomainHostdevDefNew(void) static virDomainTPMDef * virDomainTPMDefNew(virDomainXMLOption *xmlopt) { - virDomainTPMDef *def; + g_autoptr(virDomainTPMDef) def = NULL; def = g_new0(virDomainTPMDef, 1); if (xmlopt && xmlopt->privateData.tpmNew && !(def->privateData = xmlopt->privateData.tpmNew())) { - VIR_FREE(def); return NULL; } - return def; + return g_steal_pointer(&def); } void virDomainTPMDefFree(virDomainTPMDef *def) @@ -9181,7 +9179,7 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt, size_t num) { size_t i; - virDomainIdMapEntry *idmap = NULL; + g_autofree virDomainIdMapEntry *idmap = NULL; VIR_XPATH_NODE_AUTORESTORE(ctxt) idmap = g_new0(virDomainIdMapEntry, num); @@ -9193,14 +9191,13 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt, virXPathUInt("string(./@count)", ctxt, &idmap[i].count) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid idmap start/target/count settings")); - VIR_FREE(idmap); return NULL; } } g_qsort_with_data(idmap, num, sizeof(idmap[0]), virDomainIdMapEntrySort, NULL); - return idmap; + return g_steal_pointer(&idmap); } @@ -9635,20 +9632,16 @@ virDomainNetAppendIPAddress(virDomainNetDef *def, int family, unsigned int prefix) { - virNetDevIPAddr *ipDef = NULL; + g_autofree virNetDevIPAddr *ipDef = NULL; ipDef = g_new0(virNetDevIPAddr, 1); if (virSocketAddrParse(&ipDef->address, address, family) < 0) - goto error; + return -1; ipDef->prefix = prefix; VIR_APPEND_ELEMENT(def->guestIP.ips, def->guestIP.nips, ipDef); return 0; - - error: - VIR_FREE(ipDef); - return -1; } @@ -10948,16 +10941,16 @@ virDomainChrSourceDefNew(virDomainXMLOption *xmlopt) virDomainChrDef * virDomainChrDefNew(virDomainXMLOption *xmlopt) { - virDomainChrDef *def = NULL; + g_autoptr(virDomainChrDef) def = NULL; def = g_new0(virDomainChrDef, 1); def->target.port = -1; if (!(def->source = virDomainChrSourceDefNew(xmlopt))) - VIR_FREE(def); + return NULL; - return def; + return g_steal_pointer(&def); } /* Parse the XML definition for a character device @@ -12265,16 +12258,16 @@ virDomainGraphicsDefParseXMLDBus(virDomainGraphicsDef *def, virDomainGraphicsDef * virDomainGraphicsDefNew(virDomainXMLOption *xmlopt) { - virDomainGraphicsDef *def = NULL; + g_autoptr(virDomainGraphicsDef) def = NULL; def = g_new0(virDomainGraphicsDef, 1); if (xmlopt && xmlopt->privateData.graphicsNew && !(def->privateData = xmlopt->privateData.graphicsNew())) { - VIR_FREE(def); + return NULL; } - return def; + return g_steal_pointer(&def); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 81e735993d..206bb781d5 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3721,6 +3721,7 @@ int virDomainObjWaitUntil(virDomainObj *vm, void virDomainPanicDefFree(virDomainPanicDef *panic); void virDomainResourceDefFree(virDomainResourceDef *resource); void virDomainGraphicsDefFree(virDomainGraphicsDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainGraphicsDef, virDomainGraphicsDefFree); const char *virDomainInputDefGetPath(virDomainInputDef *input); void virDomainInputDefFree(virDomainInputDef *def); virDomainDiskDef *virDomainDiskDefNew(virDomainXMLOption *xmlopt); @@ -3806,6 +3807,7 @@ virDomainDeviceInfo *virDomainDeviceGetInfo(const virDomainDeviceDef *device); void virDomainDeviceSetData(virDomainDeviceDef *device, void *devicedata); void virDomainTPMDefFree(virDomainTPMDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainTPMDef, virDomainTPMDefFree); typedef int (*virDomainDeviceInfoCallback)(virDomainDef *def, virDomainDeviceDef *dev, -- 2.49.0
From: Kirill Shchetiniuk <kshcheti@redhat.com> Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/qemu/qemu_domain.c | 15 ++++++--------- src/qemu/qemu_driver.c | 6 ++---- src/qemu/qemu_migration.c | 25 +++++++++---------------- src/qemu/qemu_monitor.c | 6 ++---- 4 files changed, 19 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a42721efad..1fb469e964 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -566,7 +566,7 @@ qemuDomainMasterKeyReadFile(qemuDomainObjPrivate *priv) { g_autofree char *path = NULL; int fd = -1; - uint8_t *masterKey = NULL; + g_autofree uint8_t *masterKey = NULL; ssize_t masterKeyLen = 0; if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir))) @@ -601,7 +601,7 @@ qemuDomainMasterKeyReadFile(qemuDomainObjPrivate *priv) masterKey = g_renew(uint8_t, masterKey, masterKeyLen); - priv->masterKey = masterKey; + priv->masterKey = g_steal_pointer(&masterKey); priv->masterKeyLen = masterKeyLen; VIR_FORCE_CLOSE(fd); @@ -609,9 +609,7 @@ qemuDomainMasterKeyReadFile(qemuDomainObjPrivate *priv) return 0; error: - if (masterKeyLen > 0) - memset(masterKey, 0, masterKeyLen); - VIR_FREE(masterKey); + virSecureErase(masterKey, masterKeyLen); VIR_FORCE_CLOSE(fd); @@ -3322,7 +3320,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, { qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = config->priv; - char *monitorpath; + g_autofree char *monitorpath = NULL; g_autofree char *tmp = NULL; int n; size_t i; @@ -3349,13 +3347,12 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, switch (priv->monConfig->type) { case VIR_DOMAIN_CHR_TYPE_PTY: - priv->monConfig->data.file.path = monitorpath; + priv->monConfig->data.file.path = g_steal_pointer(&monitorpath); break; case VIR_DOMAIN_CHR_TYPE_UNIX: - priv->monConfig->data.nix.path = monitorpath; + priv->monConfig->data.nix.path = g_steal_pointer(&monitorpath); break; default: - VIR_FREE(monitorpath); virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported monitor type '%1$s'"), virDomainChrTypeToString(priv->monConfig->type)); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a1b1edcbbf..12a1b2ae9d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18858,7 +18858,7 @@ qemuDomainGetGuestVcpus(virDomainPtr dom, { virDomainObj *vm = NULL; qemuAgent *agent; - qemuAgentCPUInfo *info = NULL; + g_autofree qemuAgentCPUInfo *info = NULL; int ninfo = 0; int ret = -1; @@ -18892,7 +18892,6 @@ qemuDomainGetGuestVcpus(virDomainPtr dom, virDomainObjEndAgentJob(vm); cleanup: - VIR_FREE(info); virDomainObjEndAPI(&vm); return ret; } @@ -18906,7 +18905,7 @@ qemuDomainSetGuestVcpus(virDomainPtr dom, { virDomainObj *vm = NULL; g_autoptr(virBitmap) map = NULL; - qemuAgentCPUInfo *info = NULL; + g_autofree qemuAgentCPUInfo *info = NULL; qemuAgent *agent; int ninfo = 0; size_t i; @@ -18976,7 +18975,6 @@ qemuDomainSetGuestVcpus(virDomainPtr dom, virDomainObjEndAgentJob(vm); cleanup: - VIR_FREE(info); virDomainObjEndAPI(&vm); return ret; } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9109c4526d..fd050641c7 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3852,7 +3852,7 @@ qemuMigrationDstPrepareTunnel(virQEMUDriver *driver, static virURI * qemuMigrationAnyParseURI(const char *uri, bool *wellFormed) { - char *tmp = NULL; + g_autofree char *tmp = NULL; virURI *parsed; /* For compatibility reasons tcp://... URIs are sent as tcp:... @@ -3865,7 +3865,6 @@ qemuMigrationAnyParseURI(const char *uri, bool *wellFormed) parsed = virURIParse(uri); if (parsed && wellFormed) *wellFormed = !tmp; - VIR_FREE(tmp); return parsed; } @@ -4044,18 +4043,16 @@ qemuMigrationAnyPrepareDef(virQEMUDriver *driver, if (!(def = virDomainDefParseString(dom_xml, driver->xmlopt, qemuCaps, VIR_DOMAIN_DEF_PARSE_INACTIVE))) - goto cleanup; + return NULL; if (dname) { name = def->name; def->name = g_strdup(dname); } - cleanup: - if (def && origname) - *origname = name; - else - VIR_FREE(name); + if (origname) + *origname = g_steal_pointer(&name); + return def; } @@ -4331,7 +4328,7 @@ struct _qemuMigrationIOThread { static void qemuMigrationSrcIOFunc(void *arg) { qemuMigrationIOThread *data = arg; - char *buffer = NULL; + g_autofree char *buffer = NULL; struct pollfd fds[2]; int timeout = -1; virErrorPtr err = NULL; @@ -4409,7 +4406,6 @@ static void qemuMigrationSrcIOFunc(void *arg) goto error; VIR_FORCE_CLOSE(data->sock); - VIR_FREE(buffer); return; @@ -4428,7 +4424,6 @@ static void qemuMigrationSrcIOFunc(void *arg) if (!virLastErrorIsSystemErrno(EPIPE)) virCopyLastError(&data->err); virResetLastError(); - VIR_FREE(buffer); } @@ -4436,7 +4431,7 @@ static qemuMigrationIOThread * qemuMigrationSrcStartTunnel(virStreamPtr st, int sock) { - qemuMigrationIOThread *io = NULL; + g_autofree qemuMigrationIOThread *io = NULL; int wakeupFD[2] = { -1, -1 }; if (virPipe(wakeupFD) < 0) @@ -4459,12 +4454,11 @@ qemuMigrationSrcStartTunnel(virStreamPtr st, goto error; } - return io; + return g_steal_pointer(&io); error: VIR_FORCE_CLOSE(wakeupFD[0]); VIR_FORCE_CLOSE(wakeupFD[1]); - VIR_FREE(io); return NULL; } @@ -7131,7 +7125,7 @@ qemuMigrationSrcToLegacyFile(virQEMUDriver *driver, qemuDomainObjPrivate *priv = vm->privateData; int ret = -1; int pipeFD[2] = { -1, -1 }; - char *errbuf = NULL; + g_autofree char *errbuf = NULL; if (compressor && virPipe(pipeFD) < 0) return -1; @@ -7177,7 +7171,6 @@ qemuMigrationSrcToLegacyFile(virQEMUDriver *driver, if (errbuf) { VIR_DEBUG("Compression binary stderr: %s", NULLSTR(errbuf)); - VIR_FREE(errbuf); } return ret; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 0f1a9d13f5..b6b606963d 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -890,7 +890,7 @@ qemuMonitorInitBalloonObjectPath(qemuMonitor *mon, virDomainMemballoonDef *balloon) { ssize_t i, nprops = 0; - char *path = NULL; + g_autofree char *path = NULL; const char *name; qemuMonitorJSONListPath **bprops = NULL; @@ -970,7 +970,6 @@ qemuMonitorInitBalloonObjectPath(qemuMonitor *mon, for (i = 0; i < nprops; i++) qemuMonitorJSONListPathFree(bprops[i]); VIR_FREE(bprops); - VIR_FREE(path); return; } @@ -2341,7 +2340,7 @@ qemuMonitorMigrateToHost(qemuMonitor *mon, int port) { int ret; - char *uri = NULL; + g_autofree char *uri = NULL; VIR_DEBUG("hostname=%s port=%d flags=0x%x", hostname, port, flags); QEMU_CHECK_MONITOR(mon); @@ -2352,7 +2351,6 @@ qemuMonitorMigrateToHost(qemuMonitor *mon, ret = qemuMonitorJSONMigrate(mon, flags, uri); - VIR_FREE(uri); return ret; } -- 2.49.0
From: Kirill Shchetiniuk <kshcheti@redhat.com> Changed value var scope to enable auto memory management capailities, moved variables to closer scope. Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/qemu/qemu_capabilities.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 205bf3d0b8..562c884dac 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5416,12 +5416,12 @@ virQEMUCapsKVMSupportsNesting(void) { static char const * const kmod[] = {"kvm_intel", "kvm_amd", "kvm_hv", "kvm"}; - g_autofree char *value = NULL; - int rc; size_t i; for (i = 0; i < G_N_ELEMENTS(kmod); i++) { - VIR_FREE(value); + g_autofree char *value = NULL; + int rc; + rc = virFileReadValueString(&value, "/sys/module/%s/parameters/nested", kmod[i]); if (rc == -2) -- 2.49.0
From: Kirill Shchetiniuk <kshcheti@redhat.com> Use auto memory management, no more cleanup stage required Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/qemu/qemu_driver.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 12a1b2ae9d..6ee54f95a4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4140,19 +4140,19 @@ static int qemuDomainSetVcpusAgent(virDomainObj *vm, unsigned int nvcpus) { - qemuAgentCPUInfo *cpuinfo = NULL; + g_autofree qemuAgentCPUInfo *cpuinfo = NULL; qemuAgent *agent; int ncpuinfo; int ret = -1; if (!qemuDomainAgentAvailable(vm, true)) - goto cleanup; + return -1; if (nvcpus > virDomainDefGetVcpus(vm->def)) { virReportError(VIR_ERR_INVALID_ARG, _("requested vcpu count is greater than the count of enabled vcpus in the domain: %1$d > %2$d"), nvcpus, virDomainDefGetVcpus(vm->def)); - goto cleanup; + return -1; } agent = qemuDomainObjEnterAgent(vm); @@ -4161,21 +4161,18 @@ qemuDomainSetVcpusAgent(virDomainObj *vm, agent = NULL; if (ncpuinfo < 0) - goto cleanup; + return -1; if (qemuAgentUpdateCPUInfo(nvcpus, cpuinfo, ncpuinfo) < 0) - goto cleanup; + return -1; if (!qemuDomainAgentAvailable(vm, true)) - goto cleanup; + return -1; agent = qemuDomainObjEnterAgent(vm); ret = qemuAgentSetVCPUs(agent, cpuinfo, ncpuinfo); qemuDomainObjExitAgent(vm, agent); - cleanup: - VIR_FREE(cpuinfo); - return ret; } -- 2.49.0
From: Kirill Shchetiniuk <kshcheti@redhat.com> Moved variables to closer scope to enable suto memory management Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6ee54f95a4..369f3da69f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16354,10 +16354,7 @@ static int qemuDomainGetHostnameLease(virDomainObj *vm, char **hostname) { - char macaddr[VIR_MAC_STRING_BUFLEN]; g_autoptr(virConnect) conn = NULL; - virNetworkDHCPLeasePtr *leases = NULL; - int n_leases; size_t i, j; int ret = -1; @@ -16373,6 +16370,9 @@ qemuDomainGetHostnameLease(virDomainObj *vm, for (i = 0; i < vm->def->nnets; i++) { g_autoptr(virNetwork) network = NULL; virDomainNetDef *net = vm->def->nets[i]; + char macaddr[VIR_MAC_STRING_BUFLEN]; + g_autofree virNetworkDHCPLeasePtr *leases = NULL; + int n_leases; if (net->type != VIR_DOMAIN_NET_TYPE_NETWORK) continue; @@ -16388,15 +16388,11 @@ qemuDomainGetHostnameLease(virDomainObj *vm, goto endjob; for (j = 0; j < n_leases; j++) { - virNetworkDHCPLeasePtr lease = leases[j]; + g_autoptr(virNetworkDHCPLease) lease = leases[j]; if (lease->hostname && !*hostname) *hostname = g_strdup(lease->hostname); - - virNetworkDHCPLeaseFree(lease); } - VIR_FREE(leases); - if (*hostname) goto endjob; } -- 2.49.0
From: Kirill Shchetiniuk <kshcheti@redhat.com> Moved variables to closer scope to enbale asuto memory management Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/qemu/qemu_process.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 45fc32a663..6f656a033e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2161,9 +2161,6 @@ qemuProcessRefreshChannelVirtioState(virQEMUDriver *driver, { size_t i; int agentReason = VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_CHANNEL; - qemuMonitorChardevInfo *entry; - virObjectEvent *event = NULL; - g_autofree char *id = NULL; if (booted) agentReason = VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_DOMAIN_STARTED; @@ -2171,8 +2168,10 @@ qemuProcessRefreshChannelVirtioState(virQEMUDriver *driver, for (i = 0; i < vm->def->nchannels; i++) { virDomainChrDef *chr = vm->def->channels[i]; if (chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) { + qemuMonitorChardevInfo *entry = NULL; + virObjectEvent *event = NULL; + g_autofree char *id = NULL; - VIR_FREE(id); id = g_strdup_printf("char%s", chr->info.alias); /* port state not reported */ -- 2.49.0
From: Kirill Shchetiniuk <kshcheti@redhat.com> Defined glib cleanup function for virStream object type and enabled the automatic memory management Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/datatypes.h | 1 + src/qemu/qemu_migration.c | 14 +++++--------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/datatypes.h b/src/datatypes.h index c5a7ece786..8dd4d87572 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -746,6 +746,7 @@ struct _virStream { virFreeCallback ff; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStream, virObjectUnref); /** * _virDomainCheckpoint diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index fd050641c7..5c9aa93ac1 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5537,13 +5537,13 @@ qemuMigrationSrcPerformPeer2Peer2(virQEMUDriver *driver, qemuMigrationParams *migParams) { virDomainPtr ddomain = NULL; - char *uri_out = NULL; - char *cookie = NULL; - char *dom_xml = NULL; + g_autofree char *uri_out = NULL; + g_autofree char *cookie = NULL; + g_autofree char *dom_xml = NULL; int cookielen = 0, ret; virErrorPtr orig_err = NULL; bool cancelled; - virStreamPtr st = NULL; + g_autoptr(virStream) st = NULL; unsigned long destflags; VIR_DEBUG("driver=%p, sconn=%p, dconn=%p, vm=%p, dconnuri=%s, " @@ -5589,7 +5589,7 @@ qemuMigrationSrcPerformPeer2Peer2(virQEMUDriver *driver, if (qemuDomainObjExitRemote(vm, true) < 0) goto cleanup; } - VIR_FREE(dom_xml); + if (ret == -1) goto cleanup; @@ -5653,11 +5653,7 @@ qemuMigrationSrcPerformPeer2Peer2(virQEMUDriver *driver, ret = -1; } - virObjectUnref(st); - virErrorRestore(&orig_err); - VIR_FREE(uri_out); - VIR_FREE(cookie); return ret; } -- 2.49.0
On 11/11/25 16:01, Kirill Shchetiniuk via Devel wrote:
Refactored the code to use GLib automatic memory management capabilities. Added macros to enable the g_autoptr capability for some structures. Removed prevosly defined cleanup stages, as now it is not required. Moved some variables to closer scope, to enbale auto memory management capability for some variables.
Kirill Shchetiniuk (7): conf: Use automatic memory management qemu: Use automatic memory management virQEMUCapsKVMSupportsNesting: Use automatic memory management qemuDomainSetVcpusAgent: Use automatic memory management qemuDomainGetHostnameLease: Use automatic memory management qemuProcessRefreshChannelVirtioState: Use automatic memory management qemuMigrationSrcPerformPeer2Peer2: Use automatic memory management
src/conf/domain_conf.c | 35 +++++++++++++------------------- src/conf/domain_conf.h | 2 ++ src/datatypes.h | 1 + src/qemu/qemu_capabilities.c | 6 +++--- src/qemu/qemu_domain.c | 15 ++++++-------- src/qemu/qemu_driver.c | 33 +++++++++++------------------- src/qemu/qemu_migration.c | 39 +++++++++++++----------------------- src/qemu/qemu_monitor.c | 6 ++---- src/qemu/qemu_process.c | 7 +++---- 9 files changed, 57 insertions(+), 87 deletions(-)
code-wise, there's nothing wrong with patches, but I don't quite understand the split. I mean, in patches 3-7 you do one function at time which is okay. But then in patches 1-2 you pick up some (what looks random) functions that have not much in common. The way we usually do this kind of change is: a) you change whole file at once; this is acceptable for very small files (not on the biggest file in our repo). b) one function at once (just like you're doing in patches 2-7), c) one 'type' at once (say, you eliminate calls to virWhateverFree()) in ALL files possible). As usual, git log usually shows practice we use. Can you please split your change differently and resend? Michal
On Thu, Nov 13, 2025 at 03:58:39PM +0100, Michal Prívozník wrote:
On 11/11/25 16:01, Kirill Shchetiniuk via Devel wrote:
Refactored the code to use GLib automatic memory management capabilities. Added macros to enable the g_autoptr capability for some structures. Removed prevosly defined cleanup stages, as now it is not required. Moved some variables to closer scope, to enbale auto memory management capability for some variables.
Kirill Shchetiniuk (7): conf: Use automatic memory management qemu: Use automatic memory management virQEMUCapsKVMSupportsNesting: Use automatic memory management qemuDomainSetVcpusAgent: Use automatic memory management qemuDomainGetHostnameLease: Use automatic memory management qemuProcessRefreshChannelVirtioState: Use automatic memory management qemuMigrationSrcPerformPeer2Peer2: Use automatic memory management
src/conf/domain_conf.c | 35 +++++++++++++------------------- src/conf/domain_conf.h | 2 ++ src/datatypes.h | 1 + src/qemu/qemu_capabilities.c | 6 +++--- src/qemu/qemu_domain.c | 15 ++++++-------- src/qemu/qemu_driver.c | 33 +++++++++++------------------- src/qemu/qemu_migration.c | 39 +++++++++++++----------------------- src/qemu/qemu_monitor.c | 6 ++---- src/qemu/qemu_process.c | 7 +++---- 9 files changed, 57 insertions(+), 87 deletions(-)
code-wise, there's nothing wrong with patches, but I don't quite understand the split. I mean, in patches 3-7 you do one function at time which is okay. But then in patches 1-2 you pick up some (what looks random) functions that have not much in common.
The way we usually do this kind of change is:
a) you change whole file at once; this is acceptable for very small files (not on the biggest file in our repo). b) one function at once (just like you're doing in patches 2-7), c) one 'type' at once (say, you eliminate calls to virWhateverFree()) in ALL files possible).
As usual, git log usually shows practice we use. Can you please split your change differently and resend?
Michal
Hi Michal, Thank you for the review, I will split it and resend as the second version. Kirill
participants (2)
-
Kirill Shchetiniuk -
Michal Prívozník