[libvirt] [PATCH v4 0/4] qemu_process: Glib sponsored cleanup

older
[libvirt] [PATCH] schemas: Allow...

Daniel Henrique Barboza

19 Dec 2019 19 Dec '19

4:09 p.m.

This series got buried a few months ago. Let's go onward unto the 20s with no one left behind. changes from version 3 [1]: - rebased to master at commit 330b556829 - removed former patch 4. The 'g_strdup_printf' change was made along the road - new patch 4: I am sending this one in separate to patch 3 because this unneeded label was already in the code, being unrelated to the changes of this series. The maintainer is welcome to squash it into patch 3. [1] https://www.redhat.com/archives/libvir-list/2019-October/msg01080.html Daniel Henrique Barboza (4): qemu_process.c: use g_autofree qemu_process.c: use g_autoptr() qemu_process.c: remove cleanup labels after g_auto*() changes qemu_process.c: remove 'cleanup' label from qemuProcessCreatePretendCmd() src/qemu/qemu_process.c | 429 +++++++++++++++------------------------- 1 file changed, 157 insertions(+), 272 deletions(-) -- 2.23.0

Show replies by date

Daniel Henrique Barboza

19 Dec 19 Dec

4:09 p.m.

New subject: [libvirt] [PATCH v4 1/4] qemu_process.c: use g_autofree

Change all feasible strings and scalar pointers to use g_autofree. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 97 +++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 63 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7e1db50e8f..d6d6a9f09b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -105,7 +105,7 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, virDomainObjPtr vm) { char ebuf[1024]; - char *file = NULL; + g_autofree char *file = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -114,7 +114,6 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR) VIR_WARN("Failed to remove domain XML for %s: %s", vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf))); - VIR_FREE(file); if (priv->pidfile && unlink(priv->pidfile) < 0 && @@ -1501,7 +1500,7 @@ qemuProcessHandleBlockThreshold(qemuMonitorPtr mon G_GNUC_UNUSED, virDomainDiskDefPtr disk; virStorageSourcePtr src; unsigned int idx; - char *dev = NULL; + g_autofree char *dev = NULL; const char *path = NULL; virObjectLock(vm); @@ -1514,11 +1513,9 @@ qemuProcessHandleBlockThreshold(qemuMonitorPtr mon G_GNUC_UNUSED, if (virStorageSourceIsLocalStorage(src)) path = src->path; - if ((dev = qemuDomainDiskBackingStoreGetName(disk, src, idx))) { + if ((dev = qemuDomainDiskBackingStoreGetName(disk, src, idx))) event = virDomainEventBlockThresholdNewFromObj(vm, dev, path, threshold, excess); - VIR_FREE(dev); - } } virObjectUnlock(vm); @@ -2052,7 +2049,7 @@ static int qemuProcessReportLogError(qemuDomainLogContextPtr logCtxt, const char *msgprefix) { - char *logmsg = NULL; + g_autofree char *logmsg = NULL; size_t max; max = VIR_ERROR_MAX_LENGTH - 1; @@ -2069,7 +2066,6 @@ qemuProcessReportLogError(qemuDomainLogContextPtr logCtxt, else virReportError(VIR_ERR_INTERNAL_ERROR, _("%s: %s"), msgprefix, logmsg); - VIR_FREE(logmsg); return 0; } @@ -2089,7 +2085,7 @@ qemuProcessLookupPTYs(virDomainChrDefPtr *devices, int count, virHashTablePtr info) { - char *id = NULL; + g_autofree char *id = NULL; size_t i; int ret = -1; @@ -2098,7 +2094,7 @@ qemuProcessLookupPTYs(virDomainChrDefPtr *devices, if (chr->source->type == VIR_DOMAIN_CHR_TYPE_PTY) { qemuMonitorChardevInfoPtr entry; - VIR_FREE(id); + g_free(id); id = g_strdup_printf("char%s", chr->info.alias); entry = virHashLookup(info, id); @@ -2118,14 +2114,13 @@ qemuProcessLookupPTYs(virDomainChrDefPtr *devices, } } - VIR_FREE(chr->source->data.file.path); + g_free(chr->source->data.file.path); chr->source->data.file.path = g_strdup(entry->ptyPath); } } ret = 0; cleanup: - VIR_FREE(id); return ret; } @@ -2178,7 +2173,7 @@ qemuProcessRefreshChannelVirtioState(virQEMUDriverPtr driver, int agentReason = VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_CHANNEL; qemuMonitorChardevInfoPtr entry; virObjectEventPtr event = NULL; - char *id = NULL; + g_autofree char *id = NULL; if (booted) agentReason = VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_DOMAIN_STARTED; @@ -2204,8 +2199,6 @@ qemuProcessRefreshChannelVirtioState(virQEMUDriverPtr driver, chr->state = entry->state; } } - - VIR_FREE(id); } @@ -2622,7 +2615,7 @@ qemuProcessSetupPid(virDomainObjPtr vm, virBitmapPtr use_cpumask = NULL; virBitmapPtr afinity_cpumask = NULL; g_autoptr(virBitmap) hostcpumap = NULL; - char *mem_mask = NULL; + g_autofree char *mem_mask = NULL; int ret = -1; if ((period || quota) && @@ -2702,7 +2695,6 @@ qemuProcessSetupPid(virDomainObjPtr vm, ret = 0; cleanup: - VIR_FREE(mem_mask); if (cgroup) { if (ret < 0) virCgroupRemove(cgroup); @@ -2781,7 +2773,7 @@ qemuProcessKillManagedPRDaemon(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; virErrorPtr orig_err; - char *pidfile; + g_autofree char *pidfile = NULL; if (!(pidfile = qemuProcessBuildPRHelperPidfilePath(vm))) { VIR_WARN("Unable to construct pr-helper pidfile path"); @@ -2802,8 +2794,6 @@ qemuProcessKillManagedPRDaemon(virDomainObjPtr vm) } } virErrorRestore(&orig_err); - - VIR_FREE(pidfile); } @@ -2812,7 +2802,7 @@ qemuProcessStartPRDaemonHook(void *opaque) { virDomainObjPtr vm = opaque; size_t i, nfds = 0; - int *fds = NULL; + g_autofree int *fds = NULL; int ret = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) { @@ -2828,7 +2818,6 @@ qemuProcessStartPRDaemonHook(void *opaque) cleanup: for (i = 0; i < nfds; i++) VIR_FORCE_CLOSE(fds[i]); - VIR_FREE(fds); return ret; } @@ -2840,9 +2829,9 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm) virQEMUDriverPtr driver = priv->driver; virQEMUDriverConfigPtr cfg; int errfd = -1; - char *pidfile = NULL; + g_autofree char *pidfile = NULL; int pidfd = -1; - char *socketPath = NULL; + g_autofree char *socketPath = NULL; pid_t cpid = -1; virCommandPtr cmd = NULL; virTimeBackOffVar timebackoff; @@ -2948,9 +2937,7 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm) unlink(pidfile); } virCommandFree(cmd); - VIR_FREE(socketPath); VIR_FORCE_CLOSE(pidfd); - VIR_FREE(pidfile); VIR_FORCE_CLOSE(errfd); virObjectUnref(cfg); return ret; @@ -3366,7 +3353,7 @@ qemuProcessUpdateState(virQEMUDriverPtr driver, virDomainObjPtr vm) int oldReason; int newReason; bool running; - char *msg = NULL; + g_autofree char *msg = NULL; int ret; qemuDomainObjEnterMonitor(driver, vm); @@ -3414,7 +3401,6 @@ qemuProcessUpdateState(virQEMUDriverPtr driver, virDomainObjPtr vm) NULLSTR(msg), virDomainStateTypeToString(newState), virDomainStateReasonToString(newState, newReason)); - VIR_FREE(msg); virDomainObjSetState(vm, newState, newReason); } @@ -3879,7 +3865,7 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver, bool build) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - char *path = NULL; + g_autofree char *path = NULL; size_t i; bool shouldBuildHP = false; bool shouldBuildMB = false; @@ -3912,13 +3898,10 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver, if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm, path, build) < 0) goto cleanup; - - VIR_FREE(path); } ret = 0; cleanup: - VIR_FREE(path); virObjectUnref(cfg); return ret; } @@ -3930,7 +3913,7 @@ qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver, virDomainMemoryDefPtr mem) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - char *path = NULL; + g_autofree char *path = NULL; int ret = -1; if (qemuGetMemoryBackingPath(vm->def, cfg, mem->info.alias, &path) < 0) @@ -3944,7 +3927,6 @@ qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver, ret = 0; cleanup: - VIR_FREE(path); virObjectUnref(cfg); return ret; } @@ -4079,11 +4061,12 @@ static int qemuProcessVerifyHypervFeatures(virDomainDefPtr def, virCPUDataPtr cpu) { - char *cpuFeature; size_t i; int rc; for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { + g_autofree char *cpuFeature = NULL; + /* always supported string property */ if (i == VIR_DOMAIN_HYPERV_VENDOR_ID || i == VIR_DOMAIN_HYPERV_SPINLOCKS) @@ -4095,7 +4078,6 @@ qemuProcessVerifyHypervFeatures(virDomainDefPtr def, cpuFeature = g_strdup_printf("hv-%s", virDomainHypervTypeToString(i)); rc = virCPUDataCheckFeature(cpu, cpuFeature); - VIR_FREE(cpuFeature); if (rc < 0) { return -1; @@ -4518,17 +4500,17 @@ qemuLogOperation(virDomainObjPtr vm, virCommandPtr cmd, qemuDomainLogContextPtr logCtxt) { - char *timestamp; + g_autofree char *timestamp = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int qemuVersion = virQEMUCapsGetVersion(priv->qemuCaps); const char *package = virQEMUCapsGetPackage(priv->qemuCaps); - char *hostname = virGetHostname(); + g_autofree char *hostname = virGetHostname(); struct utsname uts; uname(&uts); if ((timestamp = virTimeStringNow()) == NULL) - goto cleanup; + return; if (qemuDomainLogContextWrite(logCtxt, "%s: %s %s, qemu version: %d.%d.%d%s, kernel: %s, hostname: %s\n", @@ -4539,17 +4521,12 @@ qemuLogOperation(virDomainObjPtr vm, NULLSTR_EMPTY(package), uts.release, NULLSTR_EMPTY(hostname)) < 0) - goto cleanup; + return; if (cmd) { - char *args = virCommandToString(cmd, true); + g_autofree char *args = virCommandToString(cmd, true); qemuDomainLogContextWrite(logCtxt, "%s\n", args); - VIR_FREE(args); } - - cleanup: - VIR_FREE(hostname); - VIR_FREE(timestamp); } @@ -4647,7 +4624,7 @@ qemuProcessStartHook(virQEMUDriverPtr driver, virHookSubopType subop) { qemuDomainObjPrivatePtr priv = vm->privateData; - char *xml; + g_autofree char *xml = NULL; int ret; if (!virHookPresent(VIR_HOOK_DRIVER_QEMU)) @@ -4658,7 +4635,6 @@ qemuProcessStartHook(virQEMUDriverPtr driver, ret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, op, subop, NULL, xml, NULL); - VIR_FREE(xml); return ret; } @@ -4771,7 +4747,7 @@ qemuProcessGetNetworkAddress(const char *netname, virSocketAddr addr; virSocketAddrPtr addrptr = NULL; char *dev_name = NULL; - char *xml = NULL; + g_autofree char *xml = NULL; *netaddr = NULL; @@ -4853,7 +4829,6 @@ qemuProcessGetNetworkAddress(const char *netname, virNetworkDefFree(netdef); virObjectUnref(net); virObjectUnref(conn); - VIR_FREE(xml); return ret; } @@ -6392,7 +6367,7 @@ qemuProcessSEVCreateFile(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; - char *configFile; + g_autofree char *configFile = NULL; int ret = -1; if (!(configFile = virFileBuildPath(priv->libDir, name, ".base64"))) @@ -6409,7 +6384,6 @@ qemuProcessSEVCreateFile(virDomainObjPtr vm, ret = 0; cleanup: - VIR_FREE(configFile); return ret; } @@ -6746,7 +6720,7 @@ qemuProcessLaunch(virConnectPtr conn, struct qemuProcessHookData hookData; virQEMUDriverConfigPtr cfg; size_t nnicindexes = 0; - int *nicindexes = NULL; + g_autofree int *nicindexes = NULL; size_t i; VIR_DEBUG("conn=%p driver=%p vm=%p name=%s if=%d asyncJob=%d " @@ -7053,7 +7027,6 @@ qemuProcessLaunch(virConnectPtr conn, virCommandFree(cmd); virObjectUnref(logCtxt); virObjectUnref(cfg); - VIR_FREE(nicindexes); return ret; } @@ -7374,7 +7347,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainDefPtr def = vm->def; const virNetDevVPortProfile *vport = NULL; size_t i; - char *timestamp; + g_autofree char *timestamp = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virConnectPtr conn = NULL; @@ -7417,7 +7390,6 @@ void qemuProcessStop(virQEMUDriverPtr driver, qemuDomainLogAppendMessage(driver, vm, "%s: shutting down, reason=%s\n", timestamp, virDomainShutoffReasonTypeToString(reason)); - VIR_FREE(timestamp); } /* Clear network bandwidth */ @@ -7488,13 +7460,12 @@ void qemuProcessStop(virQEMUDriverPtr driver, /* now that we know it's stopped call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0); + g_autofree char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0); /* we can't stop the operation even if the script raised an error */ ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END, NULL, xml, NULL)); - VIR_FREE(xml); } /* Reset Security Labels unless caller don't want us to */ @@ -7677,13 +7648,12 @@ void qemuProcessStop(virQEMUDriverPtr driver, /* The "release" hook cleans up additional resources */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0); + g_autofree char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0); /* we can't stop the operation even if the script raised an error */ virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, VIR_HOOK_QEMU_OP_RELEASE, VIR_HOOK_SUBOP_END, NULL, xml, NULL); - VIR_FREE(xml); } virDomainObjRemoveTransientDef(vm); @@ -8228,13 +8198,14 @@ qemuProcessReconnect(void *opaque) /* Run an hook to allow admins to do some magic */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, priv->qemuCaps, obj->def, 0); + g_autofree char *xml = qemuDomainDefFormatXML(driver, + priv->qemuCaps, + obj->def, 0); int hookret; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, obj->def->name, VIR_HOOK_QEMU_OP_RECONNECT, VIR_HOOK_SUBOP_BEGIN, NULL, xml, NULL); - VIR_FREE(xml); /* * If the script raised an error abort the launch @@ -8489,7 +8460,7 @@ qemuProcessQEMULabelUniqPath(qemuProcessQMPPtr proc) static int qemuProcessQMPInit(qemuProcessQMPPtr proc) { - char *template = NULL; + g_autofree char *template = NULL; VIR_DEBUG("proc=%p, emulator=%s", proc, proc->binary); -- 2.23.0

Cole Robinson

20 Dec 20 Dec

1:34 p.m.

New subject: [libvirt] [PATCH v4 1/4] qemu_process.c: use g_autofree

On 12/19/19 4:09 PM, Daniel Henrique Barboza wrote:

...

Change all feasible strings and scalar pointers to use g_autofree.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 97 +++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 63 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7e1db50e8f..d6d6a9f09b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -105,7 +105,7 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, virDomainObjPtr vm) { char ebuf[1024]; - char *file = NULL; + g_autofree char *file = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);

@@ -114,7 +114,6 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR) VIR_WARN("Failed to remove domain XML for %s: %s", vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf))); - VIR_FREE(file);

if (priv->pidfile && unlink(priv->pidfile) < 0 && @@ -1501,7 +1500,7 @@ qemuProcessHandleBlockThreshold(qemuMonitorPtr mon G_GNUC_UNUSED, virDomainDiskDefPtr disk; virStorageSourcePtr src; unsigned int idx; - char *dev = NULL; + g_autofree char *dev = NULL; const char *path = NULL;

This needs to be moved within loop scope, otherwise it will only free once at the end of the function.

...

virObjectLock(vm); @@ -1514,11 +1513,9 @@ qemuProcessHandleBlockThreshold(qemuMonitorPtr mon G_GNUC_UNUSED, if (virStorageSourceIsLocalStorage(src)) path = src->path;

- if ((dev = qemuDomainDiskBackingStoreGetName(disk, src, idx))) { + if ((dev = qemuDomainDiskBackingStoreGetName(disk, src, idx))) event = virDomainEventBlockThresholdNewFromObj(vm, dev, path, threshold, excess); - VIR_FREE(dev); - } }

virObjectUnlock(vm); @@ -2052,7 +2049,7 @@ static int qemuProcessReportLogError(qemuDomainLogContextPtr logCtxt, const char *msgprefix) { - char *logmsg = NULL; + g_autofree char *logmsg = NULL; size_t max;

max = VIR_ERROR_MAX_LENGTH - 1; @@ -2069,7 +2066,6 @@ qemuProcessReportLogError(qemuDomainLogContextPtr logCtxt, else virReportError(VIR_ERR_INTERNAL_ERROR, _("%s: %s"), msgprefix, logmsg);

- VIR_FREE(logmsg); return 0; }

@@ -2089,7 +2085,7 @@ qemuProcessLookupPTYs(virDomainChrDefPtr *devices, int count, virHashTablePtr info) { - char *id = NULL; + g_autofree char *id = NULL; size_t i; int ret = -1;

@@ -2098,7 +2094,7 @@ qemuProcessLookupPTYs(virDomainChrDefPtr *devices, if (chr->source->type == VIR_DOMAIN_CHR_TYPE_PTY) { qemuMonitorChardevInfoPtr entry;

- VIR_FREE(id); + g_free(id);

You can move id inside loop scope as well, and then this free can be dropped.

...

id = g_strdup_printf("char%s", chr->info.alias);

entry = virHashLookup(info, id); @@ -2118,14 +2114,13 @@ qemuProcessLookupPTYs(virDomainChrDefPtr *devices, } }

- VIR_FREE(chr->source->data.file.path); + g_free(chr->source->data.file.path); chr->source->data.file.path = g_strdup(entry->ptyPath); } }

ret = 0; cleanup: - VIR_FREE(id); return ret; }

@@ -2178,7 +2173,7 @@ qemuProcessRefreshChannelVirtioState(virQEMUDriverPtr driver, int agentReason = VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_CHANNEL; qemuMonitorChardevInfoPtr entry; virObjectEventPtr event = NULL; - char *id = NULL; + g_autofree char *id = NULL;

if (booted) agentReason = VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_DOMAIN_STARTED; @@ -2204,8 +2199,6 @@ qemuProcessRefreshChannelVirtioState(virQEMUDriverPtr driver, chr->state = entry->state; } } - - VIR_FREE(id); }

@@ -2622,7 +2615,7 @@ qemuProcessSetupPid(virDomainObjPtr vm, virBitmapPtr use_cpumask = NULL; virBitmapPtr afinity_cpumask = NULL; g_autoptr(virBitmap) hostcpumap = NULL; - char *mem_mask = NULL; + g_autofree char *mem_mask = NULL; int ret = -1;

if ((period || quota) && @@ -2702,7 +2695,6 @@ qemuProcessSetupPid(virDomainObjPtr vm,

ret = 0; cleanup: - VIR_FREE(mem_mask); if (cgroup) { if (ret < 0) virCgroupRemove(cgroup); @@ -2781,7 +2773,7 @@ qemuProcessKillManagedPRDaemon(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; virErrorPtr orig_err; - char *pidfile; + g_autofree char *pidfile = NULL;

if (!(pidfile = qemuProcessBuildPRHelperPidfilePath(vm))) { VIR_WARN("Unable to construct pr-helper pidfile path"); @@ -2802,8 +2794,6 @@ qemuProcessKillManagedPRDaemon(virDomainObjPtr vm) } } virErrorRestore(&orig_err); - - VIR_FREE(pidfile); }

@@ -2812,7 +2802,7 @@ qemuProcessStartPRDaemonHook(void *opaque) { virDomainObjPtr vm = opaque; size_t i, nfds = 0; - int *fds = NULL; + g_autofree int *fds = NULL; int ret = -1;

if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) { @@ -2828,7 +2818,6 @@ qemuProcessStartPRDaemonHook(void *opaque) cleanup: for (i = 0; i < nfds; i++) VIR_FORCE_CLOSE(fds[i]); - VIR_FREE(fds); return ret; }

@@ -2840,9 +2829,9 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm) virQEMUDriverPtr driver = priv->driver; virQEMUDriverConfigPtr cfg; int errfd = -1; - char *pidfile = NULL; + g_autofree char *pidfile = NULL; int pidfd = -1; - char *socketPath = NULL; + g_autofree char *socketPath = NULL; pid_t cpid = -1; virCommandPtr cmd = NULL; virTimeBackOffVar timebackoff; @@ -2948,9 +2937,7 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm) unlink(pidfile); } virCommandFree(cmd); - VIR_FREE(socketPath); VIR_FORCE_CLOSE(pidfd); - VIR_FREE(pidfile); VIR_FORCE_CLOSE(errfd); virObjectUnref(cfg); return ret; @@ -3366,7 +3353,7 @@ qemuProcessUpdateState(virQEMUDriverPtr driver, virDomainObjPtr vm) int oldReason; int newReason; bool running; - char *msg = NULL; + g_autofree char *msg = NULL; int ret;

qemuDomainObjEnterMonitor(driver, vm); @@ -3414,7 +3401,6 @@ qemuProcessUpdateState(virQEMUDriverPtr driver, virDomainObjPtr vm) NULLSTR(msg), virDomainStateTypeToString(newState), virDomainStateReasonToString(newState, newReason)); - VIR_FREE(msg); virDomainObjSetState(vm, newState, newReason); }

@@ -3879,7 +3865,7 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver, bool build) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - char *path = NULL; + g_autofree char *path = NULL; size_t i; bool shouldBuildHP = false; bool shouldBuildMB = false; @@ -3912,13 +3898,10 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver, if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm, path, build) < 0) goto cleanup; - - VIR_FREE(path); }

I think this function should get g_autofree char *path = NULL; in both the loop, and the second conditional, then all the free'ing can be dropped

...

ret = 0; cleanup: - VIR_FREE(path); virObjectUnref(cfg); return ret; } @@ -3930,7 +3913,7 @@ qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver, virDomainMemoryDefPtr mem) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - char *path = NULL; + g_autofree char *path = NULL; int ret = -1;

if (qemuGetMemoryBackingPath(vm->def, cfg, mem->info.alias, &path) < 0) @@ -3944,7 +3927,6 @@ qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver,

ret = 0; cleanup: - VIR_FREE(path); virObjectUnref(cfg); return ret; } @@ -4079,11 +4061,12 @@ static int qemuProcessVerifyHypervFeatures(virDomainDefPtr def, virCPUDataPtr cpu) { - char *cpuFeature; size_t i; int rc;

for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { + g_autofree char *cpuFeature = NULL; + /* always supported string property */ if (i == VIR_DOMAIN_HYPERV_VENDOR_ID || i == VIR_DOMAIN_HYPERV_SPINLOCKS) @@ -4095,7 +4078,6 @@ qemuProcessVerifyHypervFeatures(virDomainDefPtr def, cpuFeature = g_strdup_printf("hv-%s", virDomainHypervTypeToString(i));

rc = virCPUDataCheckFeature(cpu, cpuFeature); - VIR_FREE(cpuFeature);

if (rc < 0) { return -1; @@ -4518,17 +4500,17 @@ qemuLogOperation(virDomainObjPtr vm, virCommandPtr cmd, qemuDomainLogContextPtr logCtxt) { - char *timestamp; + g_autofree char *timestamp = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int qemuVersion = virQEMUCapsGetVersion(priv->qemuCaps); const char *package = virQEMUCapsGetPackage(priv->qemuCaps); - char *hostname = virGetHostname(); + g_autofree char *hostname = virGetHostname(); struct utsname uts;

uname(&uts);

if ((timestamp = virTimeStringNow()) == NULL) - goto cleanup; + return;

if (qemuDomainLogContextWrite(logCtxt, "%s: %s %s, qemu version: %d.%d.%d%s, kernel: %s, hostname: %s\n", @@ -4539,17 +4521,12 @@ qemuLogOperation(virDomainObjPtr vm, NULLSTR_EMPTY(package), uts.release, NULLSTR_EMPTY(hostname)) < 0) - goto cleanup; + return;

if (cmd) { - char *args = virCommandToString(cmd, true); + g_autofree char *args = virCommandToString(cmd, true); qemuDomainLogContextWrite(logCtxt, "%s\n", args); - VIR_FREE(args); } - - cleanup: - VIR_FREE(hostname); - VIR_FREE(timestamp); }

@@ -4647,7 +4624,7 @@ qemuProcessStartHook(virQEMUDriverPtr driver, virHookSubopType subop) { qemuDomainObjPrivatePtr priv = vm->privateData; - char *xml; + g_autofree char *xml = NULL; int ret;

if (!virHookPresent(VIR_HOOK_DRIVER_QEMU)) @@ -4658,7 +4635,6 @@ qemuProcessStartHook(virQEMUDriverPtr driver,

ret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, op, subop, NULL, xml, NULL); - VIR_FREE(xml);

return ret; } @@ -4771,7 +4747,7 @@ qemuProcessGetNetworkAddress(const char *netname, virSocketAddr addr; virSocketAddrPtr addrptr = NULL; char *dev_name = NULL; - char *xml = NULL; + g_autofree char *xml = NULL;

*netaddr = NULL;

@@ -4853,7 +4829,6 @@ qemuProcessGetNetworkAddress(const char *netname, virNetworkDefFree(netdef); virObjectUnref(net); virObjectUnref(conn); - VIR_FREE(xml); return ret; }

@@ -6392,7 +6367,7 @@ qemuProcessSEVCreateFile(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; - char *configFile; + g_autofree char *configFile = NULL; int ret = -1;

if (!(configFile = virFileBuildPath(priv->libDir, name, ".base64"))) @@ -6409,7 +6384,6 @@ qemuProcessSEVCreateFile(virDomainObjPtr vm,

ret = 0; cleanup: - VIR_FREE(configFile); return ret; }

@@ -6746,7 +6720,7 @@ qemuProcessLaunch(virConnectPtr conn, struct qemuProcessHookData hookData; virQEMUDriverConfigPtr cfg; size_t nnicindexes = 0; - int *nicindexes = NULL; + g_autofree int *nicindexes = NULL; size_t i;

VIR_DEBUG("conn=%p driver=%p vm=%p name=%s if=%d asyncJob=%d " @@ -7053,7 +7027,6 @@ qemuProcessLaunch(virConnectPtr conn, virCommandFree(cmd); virObjectUnref(logCtxt); virObjectUnref(cfg); - VIR_FREE(nicindexes); return ret; }

@@ -7374,7 +7347,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainDefPtr def = vm->def; const virNetDevVPortProfile *vport = NULL; size_t i; - char *timestamp; + g_autofree char *timestamp = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virConnectPtr conn = NULL;

@@ -7417,7 +7390,6 @@ void qemuProcessStop(virQEMUDriverPtr driver, qemuDomainLogAppendMessage(driver, vm, "%s: shutting down, reason=%s\n", timestamp, virDomainShutoffReasonTypeToString(reason)); - VIR_FREE(timestamp); }

/* Clear network bandwidth */ @@ -7488,13 +7460,12 @@ void qemuProcessStop(virQEMUDriverPtr driver,

/* now that we know it's stopped call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0); + g_autofree char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0);

/* we can't stop the operation even if the script raised an error */ ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END, NULL, xml, NULL)); - VIR_FREE(xml); }

/* Reset Security Labels unless caller don't want us to */ @@ -7677,13 +7648,12 @@ void qemuProcessStop(virQEMUDriverPtr driver,

/* The "release" hook cleans up additional resources */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0); + g_autofree char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0);

/* we can't stop the operation even if the script raised an error */ virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, VIR_HOOK_QEMU_OP_RELEASE, VIR_HOOK_SUBOP_END, NULL, xml, NULL); - VIR_FREE(xml); }

virDomainObjRemoveTransientDef(vm); @@ -8228,13 +8198,14 @@ qemuProcessReconnect(void *opaque)

/* Run an hook to allow admins to do some magic */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, priv->qemuCaps, obj->def, 0); + g_autofree char *xml = qemuDomainDefFormatXML(driver, + priv->qemuCaps, + obj->def, 0); int hookret;

hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, obj->def->name, VIR_HOOK_QEMU_OP_RECONNECT, VIR_HOOK_SUBOP_BEGIN, NULL, xml, NULL); - VIR_FREE(xml);

/* * If the script raised an error abort the launch @@ -8489,7 +8460,7 @@ qemuProcessQEMULabelUniqPath(qemuProcessQMPPtr proc) static int qemuProcessQMPInit(qemuProcessQMPPtr proc) { - char *template = NULL; + g_autofree char *template = NULL;

VIR_DEBUG("proc=%p, emulator=%s", proc, proc->binary);

Last one looks suspicious, either it's fixing a memory leak or something is wrong! It's a bit of both. Later code is: template = g_strdup_printf("%s/qmp-XXXXXX", proc->libDir); if (!(proc->uniqDir = g_mkdtemp(template))) { virReportSystemError(errno, _("Failed to create unique directory with " "template '%s' for probing QEMU"), template); return -1; } g_mkdtemp actually alters and returns the passed in string, it doesn't return new memory. So if g_mkdtemp succeeds, we are transfering ownership to proc->uniqDir. There's a bug though that template isn't free'd if g_mkdtemp fails. So if you convert to g_autofree, after g_mkdtemp succeeds, you need to set 'template = NULL'; - Cole

Daniel Henrique Barboza

2:02 p.m.

New subject: [libvirt] [PATCH v4 1/4] qemu_process.c: use g_autofree

On 12/20/19 3:34 PM, Cole Robinson wrote:

...

On 12/19/19 4:09 PM, Daniel Henrique Barboza wrote:

...
Change all feasible strings and scalar pointers to use g_autofree.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 97 +++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 63 deletions(-) [...]>>

Last one looks suspicious, either it's fixing a memory leak or something is wrong! It's a bit of both. Later code is:

template = g_strdup_printf("%s/qmp-XXXXXX", proc->libDir);

if (!(proc->uniqDir = g_mkdtemp(template))) {

virReportSystemError(errno,

_("Failed to create unique directory with "

"template '%s' for probing QEMU"),

template);

return -1;

}

g_mkdtemp actually alters and returns the passed in string, it doesn't return new memory. So if g_mkdtemp succeeds, we are transfering ownership to proc->uniqDir. There's a bug though that template isn't free'd if g_mkdtemp fails.

So if you convert to g_autofree, after g_mkdtemp succeeds, you need to set 'template = NULL';

Thanks. I got a weird feeling about this change (this code wasn't present in the previous version) because I thought strange that no one put a VIR_FREE() in a string returned by g_strdup_printf(), which I know for a fact that put stuff in the heap. DHB

...

- Cole

Daniel Henrique Barboza

19 Dec 19 Dec

4:09 p.m.

New subject: [libvirt] [PATCH v4 2/4] qemu_process.c: use g_autoptr()

Change all feasible pointers to use g_autoptr(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 113 +++++++++++++--------------------------- 1 file changed, 37 insertions(+), 76 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d6d6a9f09b..4870b23704 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -107,7 +107,7 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, char ebuf[1024]; g_autofree char *file = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); file = g_strdup_printf("%s/%s.xml", cfg->stateDir, vm->def->name); @@ -120,8 +120,6 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, errno != ENOENT) VIR_WARN("Failed to remove PID file for %s: %s", vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf))); - - virObjectUnref(cfg); } @@ -401,7 +399,7 @@ qemuProcessHandleReset(qemuMonitorPtr mon G_GNUC_UNUSED, virQEMUDriverPtr driver = opaque; virObjectEventPtr event; qemuDomainObjPrivatePtr priv; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); int ret = -1; virObjectLock(vm); @@ -438,7 +436,6 @@ qemuProcessHandleReset(qemuMonitorPtr mon G_GNUC_UNUSED, cleanup: virObjectUnlock(vm); virObjectEventStateQueue(driver->domainEventState, event); - virObjectUnref(cfg); return ret; } @@ -457,7 +454,7 @@ qemuProcessFakeReboot(void *opaque) virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virDomainRunningReason reason = VIR_DOMAIN_RUNNING_BOOTED; int ret = -1, rc; @@ -507,7 +504,6 @@ qemuProcessFakeReboot(void *opaque) if (ret == -1) ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_FORCE)); virDomainObjEndAPI(&vm); - virObjectUnref(cfg); } @@ -570,7 +566,7 @@ qemuProcessHandleShutdown(qemuMonitorPtr mon G_GNUC_UNUSED, virQEMUDriverPtr driver = opaque; qemuDomainObjPrivatePtr priv; virObjectEventPtr event = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); int detail = 0; VIR_DEBUG("vm=%p", vm); @@ -627,7 +623,6 @@ qemuProcessHandleShutdown(qemuMonitorPtr mon G_GNUC_UNUSED, unlock: virObjectUnlock(vm); virObjectEventStateQueue(driver->domainEventState, event); - virObjectUnref(cfg); return 0; } @@ -642,7 +637,7 @@ qemuProcessHandleStop(qemuMonitorPtr mon G_GNUC_UNUSED, virObjectEventPtr event = NULL; virDomainPausedReason reason; virDomainEventSuspendedDetailType detail; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; virObjectLock(vm); @@ -688,7 +683,6 @@ qemuProcessHandleStop(qemuMonitorPtr mon G_GNUC_UNUSED, virObjectUnlock(vm); virObjectEventStateQueue(driver->domainEventState, event); - virObjectUnref(cfg); return 0; } @@ -701,7 +695,7 @@ qemuProcessHandleResume(qemuMonitorPtr mon G_GNUC_UNUSED, { virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv; virDomainRunningReason reason = VIR_DOMAIN_RUNNING_UNPAUSED; virDomainEventResumedDetailType eventDetail; @@ -734,7 +728,6 @@ qemuProcessHandleResume(qemuMonitorPtr mon G_GNUC_UNUSED, virObjectUnlock(vm); virObjectEventStateQueue(driver->domainEventState, event); - virObjectUnref(cfg); return 0; } @@ -746,7 +739,7 @@ qemuProcessHandleRTCChange(qemuMonitorPtr mon G_GNUC_UNUSED, { virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virObjectLock(vm); @@ -778,7 +771,6 @@ qemuProcessHandleRTCChange(qemuMonitorPtr mon G_GNUC_UNUSED, virObjectUnlock(vm); virObjectEventStateQueue(driver->domainEventState, event); - virObjectUnref(cfg); return 0; } @@ -792,7 +784,7 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon G_GNUC_UNUSED, virQEMUDriverPtr driver = opaque; virObjectEventPtr watchdogEvent = NULL; virObjectEventPtr lifecycleEvent = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virObjectLock(vm); watchdogEvent = virDomainEventWatchdogNewFromObj(vm, action); @@ -840,7 +832,6 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon G_GNUC_UNUSED, virObjectEventStateQueue(driver->domainEventState, watchdogEvent); virObjectEventStateQueue(driver->domainEventState, lifecycleEvent); - virObjectUnref(cfg); return 0; } @@ -861,7 +852,7 @@ qemuProcessHandleIOError(qemuMonitorPtr mon G_GNUC_UNUSED, const char *srcPath; const char *devAlias; virDomainDiskDefPtr disk; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virObjectLock(vm); @@ -912,7 +903,6 @@ qemuProcessHandleIOError(qemuMonitorPtr mon G_GNUC_UNUSED, virObjectEventStateQueue(driver->domainEventState, ioErrorEvent); virObjectEventStateQueue(driver->domainEventState, ioErrorEvent2); virObjectEventStateQueue(driver->domainEventState, lifecycleEvent); - virObjectUnref(cfg); return 0; } @@ -1136,7 +1126,7 @@ qemuProcessHandleTrayChange(qemuMonitorPtr mon G_GNUC_UNUSED, virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; virDomainDiskDefPtr disk; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virObjectLock(vm); disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, devAlias, devid); @@ -1159,7 +1149,6 @@ qemuProcessHandleTrayChange(qemuMonitorPtr mon G_GNUC_UNUSED, virObjectUnlock(vm); virObjectEventStateQueue(driver->domainEventState, event); - virObjectUnref(cfg); return 0; } @@ -1171,7 +1160,7 @@ qemuProcessHandlePMWakeup(qemuMonitorPtr mon G_GNUC_UNUSED, virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; virObjectEventPtr lifecycleEvent = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virObjectLock(vm); event = virDomainEventPMWakeupNewFromObj(vm); @@ -1198,7 +1187,6 @@ qemuProcessHandlePMWakeup(qemuMonitorPtr mon G_GNUC_UNUSED, virObjectUnlock(vm); virObjectEventStateQueue(driver->domainEventState, event); virObjectEventStateQueue(driver->domainEventState, lifecycleEvent); - virObjectUnref(cfg); return 0; } @@ -1210,7 +1198,7 @@ qemuProcessHandlePMSuspend(qemuMonitorPtr mon G_GNUC_UNUSED, virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; virObjectEventPtr lifecycleEvent = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virObjectLock(vm); event = virDomainEventPMSuspendNewFromObj(vm); @@ -1240,7 +1228,6 @@ qemuProcessHandlePMSuspend(qemuMonitorPtr mon G_GNUC_UNUSED, virObjectEventStateQueue(driver->domainEventState, event); virObjectEventStateQueue(driver->domainEventState, lifecycleEvent); - virObjectUnref(cfg); return 0; } @@ -1252,7 +1239,7 @@ qemuProcessHandleBalloonChange(qemuMonitorPtr mon G_GNUC_UNUSED, { virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virObjectLock(vm); event = virDomainEventBalloonChangeNewFromObj(vm, actual); @@ -1267,7 +1254,6 @@ qemuProcessHandleBalloonChange(qemuMonitorPtr mon G_GNUC_UNUSED, virObjectUnlock(vm); virObjectEventStateQueue(driver->domainEventState, event); - virObjectUnref(cfg); return 0; } @@ -1279,7 +1265,7 @@ qemuProcessHandlePMSuspendDisk(qemuMonitorPtr mon G_GNUC_UNUSED, virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; virObjectEventPtr lifecycleEvent = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virObjectLock(vm); event = virDomainEventPMSuspendDiskNewFromObj(vm); @@ -1309,7 +1295,6 @@ qemuProcessHandlePMSuspendDisk(qemuMonitorPtr mon G_GNUC_UNUSED, virObjectEventStateQueue(driver->domainEventState, event); virObjectEventStateQueue(driver->domainEventState, lifecycleEvent); - virObjectUnref(cfg); return 0; } @@ -1637,7 +1622,7 @@ qemuProcessHandleMigrationStatus(qemuMonitorPtr mon G_GNUC_UNUSED, qemuDomainObjPrivatePtr priv; virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); int reason; virObjectLock(vm); @@ -1676,7 +1661,6 @@ qemuProcessHandleMigrationStatus(qemuMonitorPtr mon G_GNUC_UNUSED, cleanup: virObjectUnlock(vm); virObjectEventStateQueue(driver->domainEventState, event); - virObjectUnref(cfg); return 0; } @@ -2722,7 +2706,7 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver, { int ret = -1; size_t i = 0; - virCapsPtr caps = NULL; + g_autoptr(virCaps) caps = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; if (!vm->def->nresctrls) @@ -2753,7 +2737,6 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver, ret = 0; cleanup: - virObjectUnref(caps); return ret; } @@ -2827,7 +2810,7 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; - virQEMUDriverConfigPtr cfg; + g_autoptr(virQEMUDriverConfig) cfg = NULL; int errfd = -1; g_autofree char *pidfile = NULL; int pidfd = -1; @@ -2939,7 +2922,6 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm) virCommandFree(cmd); VIR_FORCE_CLOSE(pidfd); VIR_FORCE_CLOSE(errfd); - virObjectUnref(cfg); return ret; } @@ -2950,7 +2932,7 @@ qemuProcessInitPasswords(virQEMUDriverPtr driver, int asyncJob) { int ret = 0; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); size_t i; for (i = 0; i < vm->def->ngraphics; ++i) { @@ -2974,7 +2956,6 @@ qemuProcessInitPasswords(virQEMUDriverPtr driver, } cleanup: - virObjectUnref(cfg); return ret; } @@ -3035,7 +3016,7 @@ qemuProcessUpdateVideoRamSize(virQEMUDriverPtr driver, ssize_t i; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainVideoDefPtr video = NULL; - virQEMUDriverConfigPtr cfg = NULL; + g_autoptr(virQEMUDriverConfig) cfg = NULL; if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; @@ -3094,7 +3075,6 @@ qemuProcessUpdateVideoRamSize(virQEMUDriverPtr driver, cfg = virQEMUDriverGetConfig(driver); ret = virDomainObjSave(vm, driver->xmlopt, cfg->stateDir); - virObjectUnref(cfg); return ret; @@ -3199,7 +3179,7 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); /* Bring up netdevs before starting CPUs */ if (qemuInterfaceStartDevices(vm->def) < 0) @@ -3234,7 +3214,6 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, */ cleanup: - virObjectUnref(cfg); return ret; release: @@ -3296,7 +3275,7 @@ static void qemuProcessNotifyNets(virDomainDefPtr def) { size_t i; - virConnectPtr conn = NULL; + g_autoptr(virConnect) conn = NULL; for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; @@ -3314,8 +3293,6 @@ qemuProcessNotifyNets(virDomainDefPtr def) virDomainNetNotifyActualDevice(conn, def, net); } } - - virObjectUnref(conn); } /* Attempt to instantiate the filters. Ignore failures because it's @@ -3864,7 +3841,7 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver, virDomainMemoryDefPtr mem, bool build) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *path = NULL; size_t i; bool shouldBuildHP = false; @@ -3902,7 +3879,6 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver, ret = 0; cleanup: - virObjectUnref(cfg); return ret; } @@ -3912,7 +3888,7 @@ qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *path = NULL; int ret = -1; @@ -3927,7 +3903,6 @@ qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver, ret = 0; cleanup: - virObjectUnref(cfg); return ret; } @@ -3967,7 +3942,7 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, virDomainGraphicsDefPtr graphics, bool allocate) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); unsigned short port = 0; unsigned short tlsPort; size_t i; @@ -4052,7 +4027,6 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, ret = 0; cleanup: - virObjectUnref(cfg); return ret; } @@ -4342,7 +4316,7 @@ qemuProcessFetchCPUDefinitions(virQEMUDriverPtr driver, virDomainCapsCPUModelsPtr *cpuModels) { qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainCapsCPUModelsPtr models = NULL; + g_autoptr(virDomainCapsCPUModels) models = NULL; int rc; if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) @@ -4357,7 +4331,6 @@ qemuProcessFetchCPUDefinitions(virQEMUDriverPtr driver, return 0; error: - virObjectUnref(models); return -1; } @@ -4369,7 +4342,7 @@ qemuProcessUpdateCPU(virQEMUDriverPtr driver, { virCPUDataPtr cpu = NULL; virCPUDataPtr disabled = NULL; - virDomainCapsCPUModelsPtr models = NULL; + g_autoptr(virDomainCapsCPUModels) models = NULL; int ret = -1; /* The host CPU model comes from host caps rather than QEMU caps so @@ -4392,7 +4365,6 @@ qemuProcessUpdateCPU(virQEMUDriverPtr driver, cleanup: virCPUDataFree(cpu); virCPUDataFree(disabled); - virObjectUnref(models); return ret; } @@ -4739,9 +4711,9 @@ static int qemuProcessGetNetworkAddress(const char *netname, char **netaddr) { - virConnectPtr conn = NULL; + g_autoptr(virConnect) conn = NULL; int ret = -1; - virNetworkPtr net; + g_autoptr(virNetwork) net = NULL; virNetworkDefPtr netdef = NULL; virNetworkIPDefPtr ipdef; virSocketAddr addr; @@ -4827,8 +4799,6 @@ qemuProcessGetNetworkAddress(const char *netname, ret = 0; cleanup: virNetworkDefFree(netdef); - virObjectUnref(net); - virObjectUnref(conn); return ret; } @@ -4865,7 +4835,7 @@ qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); const char *type = virDomainGraphicsTypeToString(graphics->type); char *listenAddr = NULL; bool useSocket = false; @@ -4939,7 +4909,6 @@ qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver, ret = 0; cleanup: - virObjectUnref(cfg); return ret; } @@ -5658,7 +5627,7 @@ qemuProcessNetworkPrepareDevices(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; size_t i; - virConnectPtr conn = NULL; + g_autoptr(virConnect) conn = NULL; for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; @@ -5711,7 +5680,6 @@ qemuProcessNetworkPrepareDevices(virQEMUDriverPtr driver, } ret = 0; cleanup: - virObjectUnref(conn); return ret; } @@ -6251,7 +6219,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, int ret = -1; size_t i; qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); priv->machineName = qemuDomainGetMachineName(vm); if (!priv->machineName) @@ -6355,7 +6323,6 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, ret = 0; cleanup: - virObjectUnref(cfg); return ret; } @@ -6514,7 +6481,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, int ret = -1; unsigned int hostdev_flags = 0; qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); if (qemuPrepareNVRAM(cfg, vm) < 0) goto cleanup; @@ -6603,7 +6570,6 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, ret = 0; cleanup: - virObjectUnref(cfg); return ret; } @@ -6718,7 +6684,7 @@ qemuProcessLaunch(virConnectPtr conn, qemuDomainObjPrivatePtr priv = vm->privateData; virCommandPtr cmd = NULL; struct qemuProcessHookData hookData; - virQEMUDriverConfigPtr cfg; + g_autoptr(virQEMUDriverConfig) cfg = NULL; size_t nnicindexes = 0; g_autofree int *nicindexes = NULL; size_t i; @@ -7026,7 +6992,6 @@ qemuProcessLaunch(virConnectPtr conn, qemuDomainSecretDestroy(vm); virCommandFree(cmd); virObjectUnref(logCtxt); - virObjectUnref(cfg); return ret; } @@ -7083,8 +7048,8 @@ qemuProcessFinishStartup(virQEMUDriverPtr driver, bool startCPUs, virDomainPausedReason pausedReason) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); if (startCPUs) { VIR_DEBUG("Starting domain CPUs"); @@ -7112,7 +7077,6 @@ qemuProcessFinishStartup(virQEMUDriverPtr driver, ret = 0; cleanup: - virObjectUnref(cfg); return ret; } @@ -7348,8 +7312,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, const virNetDevVPortProfile *vport = NULL; size_t i; g_autofree char *timestamp = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - virConnectPtr conn = NULL; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virConnect) conn = NULL; VIR_DEBUG("Shutting down vm=%p name=%s id=%d pid=%lld, " "reason=%s, asyncJob=%s, flags=0x%x", @@ -7664,8 +7628,6 @@ void qemuProcessStop(virQEMUDriverPtr driver, cleanup: virErrorRestore(&orig_err); - virObjectUnref(conn); - virObjectUnref(cfg); } @@ -7979,7 +7941,7 @@ qemuProcessReconnect(void *opaque) qemuDomainJobObj oldjob; int state; int reason; - virQEMUDriverConfigPtr cfg; + g_autoptr(virQEMUDriverConfig) cfg = NULL; size_t i; unsigned int stopFlags = 0; bool jobStarted = false; @@ -8227,7 +8189,6 @@ qemuProcessReconnect(void *opaque) qemuDomainRemoveInactiveJob(driver, obj); } virDomainObjEndAPI(&obj); - virObjectUnref(cfg); virNWFilterUnlockFilterUpdates(); virIdentitySetCurrent(NULL); return; -- 2.23.0

Cole Robinson

20 Dec 20 Dec

1:47 p.m.

New subject: [libvirt] [PATCH v4 2/4] qemu_process.c: use g_autoptr()

On 12/19/19 4:09 PM, Daniel Henrique Barboza wrote:

...

Change all feasible pointers to use g_autoptr().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 113 +++++++++++++--------------------------- 1 file changed, 37 insertions(+), 76 deletions(-)

...

@@ -6718,7 +6684,7 @@ qemuProcessLaunch(virConnectPtr conn, qemuDomainObjPrivatePtr priv = vm->privateData; virCommandPtr cmd = NULL; struct qemuProcessHookData hookData; - virQEMUDriverConfigPtr cfg; + g_autoptr(virQEMUDriverConfig) cfg = NULL; size_t nnicindexes = 0; g_autofree int *nicindexes = NULL; size_t i; @@ -7026,7 +6992,6 @@ qemuProcessLaunch(virConnectPtr conn, qemuDomainSecretDestroy(vm); virCommandFree(cmd);

virCommand supports autoptr too

...

virObjectUnref(logCtxt);

Could add a prep patch in qemu_domain.h with: G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuDomainLogContext, virObjectUnref); Then use it here too - Cole

Daniel Henrique Barboza

19 Dec 19 Dec

4:09 p.m.

New subject: [libvirt] [PATCH v4 3/4] qemu_process.c: remove cleanup labels after g_auto*() changes

The g_auto*() changes made by the previous patches made a lot of 'cleanup' labels obsolete. Let's remove them. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 182 ++++++++++++++++------------------------ 1 file changed, 70 insertions(+), 112 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4870b23704..184bd6e816 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2071,7 +2071,6 @@ qemuProcessLookupPTYs(virDomainChrDefPtr *devices, { g_autofree char *id = NULL; size_t i; - int ret = -1; for (i = 0; i < count; i++) { virDomainChrDefPtr chr = devices[i]; @@ -2089,7 +2088,7 @@ qemuProcessLookupPTYs(virDomainChrDefPtr *devices, */ virReportError(VIR_ERR_INTERNAL_ERROR, _("no assigned pty for device %s"), id); - goto cleanup; + return -1; } else { /* 'info chardev' had no pty path for this chardev, * but the log output had, so we're fine @@ -2103,9 +2102,7 @@ qemuProcessLookupPTYs(virDomainChrDefPtr *devices, } } - ret = 0; - cleanup: - return ret; + return 0; } static int @@ -2704,7 +2701,6 @@ static int qemuProcessResctrlCreate(virQEMUDriverPtr driver, virDomainObjPtr vm) { - int ret = -1; size_t i = 0; g_autoptr(virCaps) caps = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -2723,7 +2719,7 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver, if (virResctrlAllocCreate(caps->host.resctrl, vm->def->resctrls[i]->alloc, priv->machineName) < 0) - goto cleanup; + return -1; for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) { virDomainResctrlMonDefPtr mon = NULL; @@ -2731,13 +2727,11 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver, mon = vm->def->resctrls[i]->monitors[j]; if (virResctrlMonitorCreate(mon->instance, priv->machineName) < 0) - goto cleanup; + return -1; } } - ret = 0; - cleanup: - return ret; + return 0; } @@ -2952,10 +2946,9 @@ qemuProcessInitPasswords(virQEMUDriverPtr driver, } if (ret < 0) - goto cleanup; + return ret; } - cleanup: return ret; } @@ -3183,7 +3176,7 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, /* Bring up netdevs before starting CPUs */ if (qemuInterfaceStartDevices(vm->def) < 0) - goto cleanup; + return -1; VIR_DEBUG("Using lock state '%s'", NULLSTR(priv->lockState)); if (virDomainLockProcessResume(driver->lockManager, cfg->uri, @@ -3192,7 +3185,7 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, * to make sure we have state still present if the user * tries to resume again */ - goto cleanup; + return -1; } VIR_FREE(priv->lockState); @@ -3213,7 +3206,6 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, * lifecycle event. */ - cleanup: return ret; release: @@ -3221,7 +3213,7 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, if (virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState) < 0) VIR_WARN("Unable to release lease on %s", vm->def->name); VIR_DEBUG("Preserving lock state '%s'", NULLSTR(priv->lockState)); - goto cleanup; + return ret; } @@ -3846,7 +3838,6 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver, size_t i; bool shouldBuildHP = false; bool shouldBuildMB = false; - int ret = -1; if (build) { shouldBuildHP = qemuProcessNeedHugepagesPath(vm->def, mem); @@ -3858,11 +3849,11 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver, path = qemuGetDomainHugepagePath(vm->def, &cfg->hugetlbfs[i]); if (!path) - goto cleanup; + return -1; if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm, path, build) < 0) - goto cleanup; + return -1; VIR_FREE(path); } @@ -3870,16 +3861,14 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver, if (!build || shouldBuildMB) { if (qemuGetMemoryBackingDomainPath(vm->def, cfg, &path) < 0) - goto cleanup; + return -1; if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm, path, build) < 0) - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } @@ -3890,20 +3879,17 @@ qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver, { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *path = NULL; - int ret = -1; if (qemuGetMemoryBackingPath(vm->def, cfg, mem->info.alias, &path) < 0) - goto cleanup; + return -1; if (unlink(path) < 0 && errno != ENOENT) { virReportSystemError(errno, _("Unable to remove %s"), path); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } @@ -3947,7 +3933,6 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, unsigned short tlsPort; size_t i; int defaultMode = graphics->data.spice.defaultMode; - int ret = -1; bool needTLSPort = false; bool needPort = false; @@ -3993,13 +3978,12 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, if (needTLSPort || graphics->data.spice.tlsPort == -1) graphics->data.spice.tlsPort = 5902; - ret = 0; - goto cleanup; + return 0; } if (needPort || graphics->data.spice.port == -1) { if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0) - goto cleanup; + return -1; graphics->data.spice.port = port; @@ -4012,11 +3996,11 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Auto allocation of spice TLS port requested " "but spice TLS is disabled in qemu.conf")); - goto cleanup; + return -1; } if (virPortAllocatorAcquire(driver->remotePorts, &tlsPort) < 0) - goto cleanup; + return -1; graphics->data.spice.tlsPort = tlsPort; @@ -4024,10 +4008,7 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, graphics->data.spice.tlsPortReserved = true; } - ret = 0; - - cleanup: - return ret; + return 0; } @@ -4320,18 +4301,15 @@ qemuProcessFetchCPUDefinitions(virQEMUDriverPtr driver, int rc; if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) - goto error; + return -1; rc = virQEMUCapsFetchCPUModels(priv->mon, vm->def->os.arch, &models); if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) - goto error; + return -1; *cpuModels = models; return 0; - - error: - return -1; } @@ -4840,7 +4818,6 @@ qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver, char *listenAddr = NULL; bool useSocket = false; size_t i; - int ret = -1; switch (graphics->type) { case VIR_DOMAIN_GRAPHICS_TYPE_VNC: @@ -4889,7 +4866,7 @@ qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver, if (qemuProcessGraphicsSetupNetworkAddress(glisten, listenAddr) < 0) - goto cleanup; + return -1; break; case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET: @@ -4906,10 +4883,7 @@ qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver, } } - ret = 0; - - cleanup: - return ret; + return 0; } @@ -5625,7 +5599,6 @@ qemuProcessNetworkPrepareDevices(virQEMUDriverPtr driver, { virDomainDefPtr def = vm->def; qemuDomainObjPrivatePtr priv = vm->privateData; - int ret = -1; size_t i; g_autoptr(virConnect) conn = NULL; @@ -5639,9 +5612,9 @@ qemuProcessNetworkPrepareDevices(virQEMUDriverPtr driver, */ if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (!conn && !(conn = virGetConnectNetwork())) - goto cleanup; + return -1; if (virDomainNetAllocateActualDevice(conn, def, net) < 0) - goto cleanup; + return -1; } actualType = virDomainNetGetActualType(net); @@ -5665,10 +5638,10 @@ qemuProcessNetworkPrepareDevices(virQEMUDriverPtr driver, pcisrc->addr.domain, pcisrc->addr.bus, pcisrc->addr.slot, pcisrc->addr.function, net->data.network.name, def->name); - goto cleanup; + return -1; } if (virDomainHostdevInsert(def, hostdev) < 0) - goto cleanup; + return -1; } else if (actualType == VIR_DOMAIN_NET_TYPE_USER && !priv->disableSlirp && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { @@ -5678,9 +5651,7 @@ qemuProcessNetworkPrepareDevices(virQEMUDriverPtr driver, } } - ret = 0; - cleanup: - return ret; + return 0; } @@ -6216,14 +6187,13 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned int flags) { - int ret = -1; size_t i; qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); priv->machineName = qemuDomainGetMachineName(vm); if (!priv->machineName) - goto cleanup; + return -1; if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { /* If you are using a SecurityDriver with dynamic labelling, @@ -6231,12 +6201,12 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, VIR_DEBUG("Generating domain security label (if required)"); if (qemuSecurityGenLabel(driver->securityManager, vm->def) < 0) { virDomainAuditSecurityLabel(vm, false); - goto cleanup; + return -1; } virDomainAuditSecurityLabel(vm, true); if (qemuProcessPrepareDomainNUMAPlacement(driver, vm) < 0) - goto cleanup; + return -1; } /* Whether we should use virtlogd as stdio handler for character @@ -6261,53 +6231,53 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, VIR_DEBUG("Assigning domain PCI addresses"); if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, driver, vm, !!(flags & VIR_QEMU_PROCESS_START_NEW))) < 0) { - goto cleanup; + return -1; } if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) - goto cleanup; + return -1; VIR_DEBUG("Setting graphics devices"); if (qemuProcessSetupGraphics(driver, vm, priv->qemuCaps, flags) < 0) - goto cleanup; + return -1; VIR_DEBUG("Create domain masterKey"); if (qemuDomainMasterKeyCreate(vm) < 0) - goto cleanup; + return -1; VIR_DEBUG("Setting up storage"); if (qemuProcessPrepareDomainStorage(driver, vm, priv, cfg, flags) < 0) - goto cleanup; + return -1; VIR_DEBUG("Prepare chardev source backends for TLS"); qemuDomainPrepareChardevSource(vm->def, cfg); VIR_DEBUG("Prepare device secrets"); if (qemuDomainSecretPrepare(driver, vm) < 0) - goto cleanup; + return -1; VIR_DEBUG("Prepare bios/uefi paths"); if (qemuFirmwareFillDomain(driver, vm, flags) < 0) - goto cleanup; + return -1; if (qemuDomainInitializePflashStorageSource(vm) < 0) - goto cleanup; + return -1; VIR_DEBUG("Preparing external devices"); if (qemuExtDevicesPrepareDomain(driver, vm) < 0) - goto cleanup; + return -1; for (i = 0; i < vm->def->nchannels; i++) { if (qemuDomainPrepareChannel(vm->def->channels[i], priv->channelTargetDir) < 0) - goto cleanup; + return -1; } if (!(priv->monConfig = virDomainChrSourceDefNew(driver->xmlopt))) - goto cleanup; + return -1; VIR_DEBUG("Preparing monitor state"); if (qemuProcessPrepareMonitorChr(priv->monConfig, priv->libDir) < 0) - goto cleanup; + return -1; priv->monError = false; priv->monStart = 0; @@ -6316,14 +6286,12 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, VIR_DEBUG("Updating guest CPU definition"); if (qemuProcessUpdateGuestCPU(vm->def, priv->qemuCaps, driver->hostarch, flags) < 0) - goto cleanup; + return -1; for (i = 0; i < vm->def->nshmems; i++) qemuDomainPrepareShmemChardev(vm->def->shmems[i]); - ret = 0; - cleanup: - return ret; + return 0; } @@ -6335,7 +6303,6 @@ qemuProcessSEVCreateFile(virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; g_autofree char *configFile = NULL; - int ret = -1; if (!(configFile = virFileBuildPath(priv->libDir, name, ".base64"))) return -1; @@ -6343,15 +6310,13 @@ qemuProcessSEVCreateFile(virDomainObjPtr vm, if (virFileRewriteStr(configFile, S_IRUSR | S_IWUSR, data) < 0) { virReportSystemError(errno, _("failed to write data to config '%s'"), configFile); - goto cleanup; + return -1; } if (qemuSecurityDomainSetPathLabel(driver, vm, configFile, true) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } @@ -6478,17 +6443,16 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned int flags) { - int ret = -1; unsigned int hostdev_flags = 0; qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); if (qemuPrepareNVRAM(cfg, vm) < 0) - goto cleanup; + return -1; if (vm->def->vsock) { if (qemuProcessOpenVhostVsock(vm->def->vsock) < 0) - goto cleanup; + return -1; } /* network devices must be "prepared" before hostdevs, because * setting up a network device might create a new hostdev that @@ -6496,7 +6460,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, */ VIR_DEBUG("Preparing network devices"); if (qemuProcessNetworkPrepareDevices(driver, vm) < 0) - goto cleanup; + return -1; /* Must be run before security labelling */ VIR_DEBUG("Preparing host devices"); @@ -6506,17 +6470,17 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, hostdev_flags |= VIR_HOSTDEV_COLD_BOOT; if (qemuHostdevPrepareDomainDevices(driver, vm->def, priv->qemuCaps, hostdev_flags) < 0) - goto cleanup; + return -1; VIR_DEBUG("Preparing chr devices"); if (virDomainChrDefForeach(vm->def, true, qemuProcessPrepareChardevDevice, NULL) < 0) - goto cleanup; + return -1; if (qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, true) < 0) - goto cleanup; + return -1; /* Ensure no historical cgroup for this VM is lying around bogus * settings */ @@ -6527,14 +6491,14 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, virReportSystemError(errno, _("cannot create log directory %s"), cfg->logDir); - goto cleanup; + return -1; } VIR_FREE(priv->pidfile); if (!(priv->pidfile = virPidFileBuildPath(cfg->stateDir, vm->def->name))) { virReportSystemError(errno, "%s", _("Failed to build pidfile path.")); - goto cleanup; + return -1; } if (unlink(priv->pidfile) < 0 && @@ -6542,7 +6506,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, virReportSystemError(errno, _("Cannot remove stale PID file %s"), priv->pidfile); - goto cleanup; + return -1; } /* @@ -6551,26 +6515,24 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, */ if (qemuProcessMakeDir(driver, vm, priv->libDir) < 0 || qemuProcessMakeDir(driver, vm, priv->channelTargetDir) < 0) - goto cleanup; + return -1; VIR_DEBUG("Write domain masterKey"); if (qemuDomainWriteMasterKeyFile(driver, vm) < 0) - goto cleanup; + return -1; VIR_DEBUG("Preparing disks (host)"); if (qemuProcessPrepareHostStorage(driver, vm, flags) < 0) - goto cleanup; + return -1; VIR_DEBUG("Preparing external devices"); if (qemuExtDevicesPrepareHost(driver, vm) < 0) - goto cleanup; + return -1; if (qemuProcessPrepareSEVGuestInput(vm) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } @@ -7048,7 +7010,6 @@ qemuProcessFinishStartup(virQEMUDriverPtr driver, bool startCPUs, virDomainPausedReason pausedReason) { - int ret = -1; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); if (startCPUs) { @@ -7059,7 +7020,7 @@ qemuProcessFinishStartup(virQEMUDriverPtr driver, if (virGetLastErrorCode() == VIR_ERR_OK) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("resume operation failed")); - goto cleanup; + return -1; } } else { virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, pausedReason); @@ -7067,17 +7028,14 @@ qemuProcessFinishStartup(virQEMUDriverPtr driver, VIR_DEBUG("Writing domain status to disk"); if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto cleanup; + return -1; if (qemuProcessStartHook(driver, vm, VIR_HOOK_QEMU_OP_STARTED, VIR_HOOK_SUBOP_BEGIN) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - return ret; + return 0; } -- 2.23.0

Daniel Henrique Barboza

4:09 p.m.

New subject: [libvirt] [PATCH v4 4/4] qemu_process.c: remove 'cleanup' label from qemuProcessCreatePretendCmd()

The 'cleanup' flag is doing no cleaup in this function. We can remove it and return NULL on error or qemuBuildCommandLine(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- As mentioned in the cover, sending this one in its own patch simply because it is not related to the cleanup made in this series - I spotted by code inspection. The maintainer is welcome to squash this one in the previous patch. src/qemu/qemu_process.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 184bd6e816..2a629c0de7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7152,11 +7152,9 @@ qemuProcessCreatePretendCmd(virQEMUDriverPtr driver, bool standalone, unsigned int flags) { - virCommandPtr cmd = NULL; - - virCheckFlagsGoto(VIR_QEMU_PROCESS_START_COLD | - VIR_QEMU_PROCESS_START_PAUSED | - VIR_QEMU_PROCESS_START_AUTODESTROY, cleanup); + virCheckFlags(VIR_QEMU_PROCESS_START_COLD | + VIR_QEMU_PROCESS_START_PAUSED | + VIR_QEMU_PROCESS_START_AUTODESTROY, NULL); flags |= VIR_QEMU_PROCESS_START_PRETEND; flags |= VIR_QEMU_PROCESS_START_NEW; @@ -7165,26 +7163,23 @@ qemuProcessCreatePretendCmd(virQEMUDriverPtr driver, if (qemuProcessInit(driver, vm, NULL, QEMU_ASYNC_JOB_NONE, !!migrateURI, flags) < 0) - goto cleanup; + return NULL; if (qemuProcessPrepareDomain(driver, vm, flags) < 0) - goto cleanup; + return NULL; VIR_DEBUG("Building emulator command line"); - cmd = qemuBuildCommandLine(driver, - NULL, - driver->securityManager, - vm, - migrateURI, - NULL, - VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, - standalone, - enableFips, - NULL, - NULL); - - cleanup: - return cmd; + return qemuBuildCommandLine(driver, + NULL, + driver->securityManager, + vm, + migrateURI, + NULL, + VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, + standalone, + enableFips, + NULL, + NULL); } -- 2.23.0

Cole Robinson

20 Dec 20 Dec

1:57 p.m.

On 12/19/19 4:09 PM, Daniel Henrique Barboza wrote:

...

This series got buried a few months ago. Let's go onward unto the 20s with no one left behind.

changes from version 3 [1]: - rebased to master at commit 330b556829 - removed former patch 4. The 'g_strdup_printf' change was made along the road - new patch 4: I am sending this one in separate to patch 3 because this unneeded label was already in the code, being unrelated to the changes of this series. The maintainer is welcome to squash it into patch 3.

[1] https://www.redhat.com/archives/libvir-list/2019-October/msg01080.html

Daniel Henrique Barboza (4): qemu_process.c: use g_autofree qemu_process.c: use g_autoptr() qemu_process.c: remove cleanup labels after g_auto*() changes qemu_process.c: remove 'cleanup' label from qemuProcessCreatePretendCmd()

src/qemu/qemu_process.c | 429 +++++++++++++++------------------------- 1 file changed, 157 insertions(+), 272 deletions(-)

Patch 3 and 4 look fine, some comments on the first two. I can't really decide what is the best way to approach cleanups like this. Should patches be split by function, and do all cleanups in one pass, or do one type of cleanup, but over a larger surface per file? Like you have done here. The first method is more tedious, but it's easier for reviewers, and good patches can go in first while patches with issues can be kicked out for v2. But it could be thousands of patches judging by the 3000+ cleanup labels we have in the code base, which sounds extreme. I think in general you will find the list more receptive to series of small per function patches though. Optimizing for ease of review will always give things a better chance of getting committed in my experience. This is just recommendation for future series though, I'll review this one as is - Cole

Daniel Henrique Barboza

2:31 p.m.

On 12/20/19 3:57 PM, Cole Robinson wrote:

...

On 12/19/19 4:09 PM, Daniel Henrique Barboza wrote:

...
This series got buried a few months ago. Let's go onward unto the 20s with no one left behind.

changes from version 3 [1]: - rebased to master at commit 330b556829 - removed former patch 4. The 'g_strdup_printf' change was made along the road - new patch 4: I am sending this one in separate to patch 3 because this unneeded label was already in the code, being unrelated to the changes of this series. The maintainer is welcome to squash it into patch 3.

[1] https://www.redhat.com/archives/libvir-list/2019-October/msg01080.html

Daniel Henrique Barboza (4): qemu_process.c: use g_autofree qemu_process.c: use g_autoptr() qemu_process.c: remove cleanup labels after g_auto*() changes qemu_process.c: remove 'cleanup' label from qemuProcessCreatePretendCmd()

src/qemu/qemu_process.c | 429 +++++++++++++++------------------------- 1 file changed, 157 insertions(+), 272 deletions(-)

Patch 3 and 4 look fine, some comments on the first two.

I can't really decide what is the best way to approach cleanups like this. Should patches be split by function, and do all cleanups in one pass, or do one type of cleanup, but over a larger surface per file? Like you have done here.

This cleanup strategy was proposed by Jano [1] a few months ago in another cleanup [2]. This is what he proposed: ---- These patches would look much better split by: * individual functions (in case you do rework multiple things at once) * individual changes, i.e. * g_autofree for scalars * g_autoptr for pointers and unref * possible removal of cleanup labels Especially splitting the goto -> return change makes the patches much more easier to read, since it makes it obvious that you don't change the exit points of the function while adding the autofree attributes. ---- He followed up with: ----

...

Do you mean sending an individual patch for any function that might have, say, 2+ changes in it? For example, if the same function was changed to use g_autoptr and g_autofree and perhaps that causes a label to be dropped, this can be an individual patch?

Yes, but if you convert a lot of functions, that would result in a lot of patches. Sending one patch per function is more viable for the cases where you need to refactor it in order to add some functionality later (see Jirka's series for example). For mass conversion for the sake of conversion, one patch per change is better. ---- These cleanups are cleanups for the sake of cleanups, thus I followed up with this model of one type of change across all the file per patch. [1] https://www.redhat.com/archives/libvir-list/2019-October/msg00985.html [2] https://www.redhat.com/archives/libvir-list/2019-October/msg00918.html

...

The first method is more tedious, but it's easier for reviewers, and good patches can go in first while patches with issues can be kicked out for v2. But it could be thousands of patches judging by the 3000+ cleanup labels we have in the code base, which sounds extreme.

I think in general you will find the list more receptive to series of small per function patches though. Optimizing for ease of review will always give things a better chance of getting committed in my experience. This is just recommendation for future series though, I'll review this one as is

I honestly don't have an option about which method is better. I agree that splitting one function per patch is easier for review. I don't mind the effort into splitting this work either. So, what I'll do then is to apply your reviews in this same format, then I'll experiment with how much individual patches this would generate. If it's too extreme (I have no idea how much functions I touched overall in this file) then I'll resubmit in the old format. If it's a feasible number I'll send it one per function. Perhaps a hybrid - similar functions that got changed in the same way can go in the same patch. I'll experiment. There are more cleanups to be made in the future. As soon as we can get down to a feasible format for them, better. Thanks, DHB

...

- Cole

Cole Robinson

2:42 p.m.

On 12/20/19 2:31 PM, Daniel Henrique Barboza wrote:

...

On 12/20/19 3:57 PM, Cole Robinson wrote:

...
On 12/19/19 4:09 PM, Daniel Henrique Barboza wrote:

...
This series got buried a few months ago. Let's go onward unto the 20s with no one left behind.

changes from version 3 [1]: - rebased to master at commit 330b556829 - removed former patch 4. The 'g_strdup_printf' change was made along the road - new patch 4: I am sending this one in separate to patch 3 because this unneeded label was already in the code, being unrelated to the changes of this series. The maintainer is welcome to squash it into patch 3.

[1] https://www.redhat.com/archives/libvir-list/2019-October/msg01080.html

Daniel Henrique Barboza (4): qemu_process.c: use g_autofree qemu_process.c: use g_autoptr() qemu_process.c: remove cleanup labels after g_auto*() changes qemu_process.c: remove 'cleanup' label from qemuProcessCreatePretendCmd()

src/qemu/qemu_process.c | 429 +++++++++++++++------------------------- 1 file changed, 157 insertions(+), 272 deletions(-)

Patch 3 and 4 look fine, some comments on the first two.

I can't really decide what is the best way to approach cleanups like this. Should patches be split by function, and do all cleanups in one pass, or do one type of cleanup, but over a larger surface per file? Like you have done here.

This cleanup strategy was proposed by Jano [1] a few months ago in another cleanup [2]. This is what he proposed:

---- These patches would look much better split by: * individual functions (in case you do rework multiple things at once) * individual changes, i.e. * g_autofree for scalars * g_autoptr for pointers and unref * possible removal of cleanup labels

Especially splitting the goto -> return change makes the patches much more easier to read, since it makes it obvious that you don't change the exit points of the function while adding the autofree attributes. ----

He followed up with:

----

...
Do you mean sending an individual patch for any function that might have, say, 2+ changes in it? For example, if the same function was changed to use g_autoptr and g_autofree and perhaps that causes a label to be dropped, this can be an individual patch?

Yes, but if you convert a lot of functions, that would result in a lot of patches.

Sending one patch per function is more viable for the cases where you need to refactor it in order to add some functionality later (see Jirka's series for example). For mass conversion for the sake of conversion, one patch per change is better. ----

These cleanups are cleanups for the sake of cleanups, thus I followed up with this model of one type of change across all the file per patch.

[1] https://www.redhat.com/archives/libvir-list/2019-October/msg00985.html [2] https://www.redhat.com/archives/libvir-list/2019-October/msg00918.html

Ah I didn't realize (or forgot). I'll defer to Jano on this one, he's done far more reviews than me. So I suggest sticking with his method Thanks, Cole

Michal Prívozník

3:54 p.m.

On 12/20/19 7:57 PM, Cole Robinson wrote:

...

On 12/19/19 4:09 PM, Daniel Henrique Barboza wrote:

...
This series got buried a few months ago. Let's go onward unto the 20s with no one left behind.

changes from version 3 [1]: - rebased to master at commit 330b556829 - removed former patch 4. The 'g_strdup_printf' change was made along the road - new patch 4: I am sending this one in separate to patch 3 because this unneeded label was already in the code, being unrelated to the changes of this series. The maintainer is welcome to squash it into patch 3.

[1] https://www.redhat.com/archives/libvir-list/2019-October/msg01080.html

Daniel Henrique Barboza (4): qemu_process.c: use g_autofree qemu_process.c: use g_autoptr() qemu_process.c: remove cleanup labels after g_auto*() changes qemu_process.c: remove 'cleanup' label from qemuProcessCreatePretendCmd()

src/qemu/qemu_process.c | 429 +++++++++++++++------------------------- 1 file changed, 157 insertions(+), 272 deletions(-)

Patch 3 and 4 look fine, some comments on the first two.

I can't really decide what is the best way to approach cleanups like this. Should patches be split by function, and do all cleanups in one pass, or do one type of cleanup, but over a larger surface per file? Like you have done here.

The first method is more tedious, but it's easier for reviewers, and good patches can go in first while patches with issues can be kicked out for v2. But it could be thousands of patches judging by the 3000+ cleanup labels we have in the code base, which sounds extreme.

I think in general you will find the list more receptive to series of small per function patches though. Optimizing for ease of review will always give things a better chance of getting committed in my experience. This is just recommendation for future series though, I'll review this one as is

I think the sooner we get this over with the better (i.e. less rebase conflicts). It's like tearing off a patch (bandage?) - it hurts less if you do it all at once. Having said that, it still makes sense to honour our rules and have one semantic chnage per commit. Michal

Cole Robinson

4:25 p.m.

On 12/20/19 3:54 PM, Michal Prívozník wrote:

...

On 12/20/19 7:57 PM, Cole Robinson wrote:

...
On 12/19/19 4:09 PM, Daniel Henrique Barboza wrote:

...
This series got buried a few months ago. Let's go onward unto the 20s with no one left behind.

changes from version 3 [1]: - rebased to master at commit 330b556829 - removed former patch 4. The 'g_strdup_printf' change was made along the road - new patch 4: I am sending this one in separate to patch 3 because this unneeded label was already in the code, being unrelated to the changes of this series. The maintainer is welcome to squash it into patch 3.

[1] https://www.redhat.com/archives/libvir-list/2019-October/msg01080.html

Daniel Henrique Barboza (4): qemu_process.c: use g_autofree qemu_process.c: use g_autoptr() qemu_process.c: remove cleanup labels after g_auto*() changes qemu_process.c: remove 'cleanup' label from qemuProcessCreatePretendCmd()

src/qemu/qemu_process.c | 429 +++++++++++++++------------------------- 1 file changed, 157 insertions(+), 272 deletions(-)

Patch 3 and 4 look fine, some comments on the first two.

I can't really decide what is the best way to approach cleanups like this. Should patches be split by function, and do all cleanups in one pass, or do one type of cleanup, but over a larger surface per file? Like you have done here.

The first method is more tedious, but it's easier for reviewers, and good patches can go in first while patches with issues can be kicked out for v2. But it could be thousands of patches judging by the 3000+ cleanup labels we have in the code base, which sounds extreme.

I think in general you will find the list more receptive to series of small per function patches though. Optimizing for ease of review will always give things a better chance of getting committed in my experience. This is just recommendation for future series though, I'll review this one as is

I think the sooner we get this over with the better (i.e. less rebase conflicts). It's like tearing off a patch (bandage?) - it hurts less if you do it all at once.

Yes I agree. Dropping the ALLOC < 0 bits shouldn't be done incrementally. It should be a largely mechanical change, everything under the 'if' is already dead code. - Cole

2088

Age (days ago)

2089

Last active (days ago)

List overview

Download

12 comments

3 participants

participants (3)

Cole Robinson
Daniel Henrique Barboza
Michal Prívozník

[libvirt] [PATCH v4 0/4] qemu_process: Glib sponsored cleanup

tags

participants (3)