[libvirt] [PATCH v3 06/10] qemu_driver: use VIR_AUTOFREE() with strings 1/3
Daniel Henrique Barboza
danielhb413 at gmail.com
Wed Oct 16 14:14:05 UTC 2019
On 10/16/19 10:49 AM, Ján Tomko wrote:
> On Tue, Oct 15, 2019 at 05:08:48PM -0300, Daniel Henrique Barboza wrote:
>> Using VIR_AUTOFREE() in all strings of qemu_driver.c make the code
>> a bit tidier and smaller, sparing VIR_FREE() calls and sometimes a
>> whole 'cleanup' label.
>
> These patches would look much better split by:
> * individual functions (in case you do rework multiple things at once)
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?
> * individual changes, i.e.
> * g_autofree for scalars
> * g_autoptr for pointers and unref
> * possible removal of cleanup labels
>
Got it. I'll reorganize the changes and send it this way.
> 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.
>
> Jano
>
>>
>> This is a huge change due to the amount of char * declared in
>> this file, thus let's split it in 3. This is the first part.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>> ---
>> src/qemu/qemu_driver.c | 103 +++++++++++++----------------------------
>> 1 file changed, 33 insertions(+), 70 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index d95c5c5b81..f887a79ecd 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -381,11 +381,9 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>> void *data)
>> {
>> char *baseDir = (char *)data;
>> - char *snapDir = NULL;
>> + VIR_AUTOFREE(char *) snapDir = NULL;
>> DIR *dir = NULL;
>> struct dirent *entry;
>> - char *xmlStr;
>> - char *fullpath;
>> virDomainSnapshotDefPtr def = NULL;
>> virDomainMomentObjPtr snap = NULL;
>> virDomainMomentObjPtr current = NULL;
>> @@ -420,6 +418,9 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>> goto cleanup;
>>
>> while ((direrr = virDirRead(dir, &entry, NULL)) > 0) {
>> + VIR_AUTOFREE(char *) xmlStr = NULL;
>> + VIR_AUTOFREE(char *) fullpath = NULL;
>> +
>> /* NB: ignoring errors, so one malformed config doesn't
>> kill the whole process */
>> VIR_INFO("Loading snapshot file '%s'", entry->d_name);
>> @@ -435,7 +436,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>> virReportSystemError(errno,
>> _("Failed to read snapshot file %s"),
>> fullpath);
>> - VIR_FREE(fullpath);
>> continue;
>> }
>>
>> @@ -448,8 +448,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> _("Failed to parse snapshot XML from file
>> '%s'"),
>> fullpath);
>> - VIR_FREE(fullpath);
>> - VIR_FREE(xmlStr);
>> continue;
>> }
>>
>> @@ -463,9 +461,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>> vm->def->name);
>> current = snap;
>> }
>> -
>> - VIR_FREE(fullpath);
>> - VIR_FREE(xmlStr);
>> }
>> if (direrr < 0)
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -492,7 +487,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>> ret = 0;
>> cleanup:
>> VIR_DIR_CLOSE(dir);
>> - VIR_FREE(snapDir);
>> virObjectUnlock(vm);
>> return ret;
>> }
>> @@ -503,11 +497,9 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
>> void *data)
>> {
>> char *baseDir = (char *)data;
>> - char *chkDir = NULL;
>> + VIR_AUTOFREE(char *) chkDir = NULL;
>> DIR *dir = NULL;
>> struct dirent *entry;
>> - char *xmlStr;
>> - char *fullpath;
>> virDomainCheckpointDefPtr def = NULL;
>> virDomainMomentObjPtr chk = NULL;
>> virDomainMomentObjPtr current = NULL;
>> @@ -538,6 +530,9 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
>> goto cleanup;
>>
>> while ((direrr = virDirRead(dir, &entry, NULL)) > 0) {
>> + VIR_AUTOFREE(char *) xmlStr = NULL;
>> + VIR_AUTOFREE(char *) fullpath = NULL;
>> +
>> /* NB: ignoring errors, so one malformed config doesn't
>> kill the whole process */
>> VIR_INFO("Loading checkpoint file '%s'", entry->d_name);
>> @@ -553,7 +548,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
>> virReportSystemError(errno,
>> _("Failed to read checkpoint file %s"),
>> fullpath);
>> - VIR_FREE(fullpath);
>> continue;
>> }
>>
>> @@ -566,8 +560,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> _("Failed to parse checkpoint XML from
>> file '%s'"),
>> fullpath);
>> - VIR_FREE(fullpath);
>> - VIR_FREE(xmlStr);
>> virObjectUnref(def);
>> continue;
>> }
>> @@ -575,9 +567,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
>> chk = virDomainCheckpointAssignDef(vm->checkpoints, def);
>> if (chk == NULL)
>> virObjectUnref(def);
>> -
>> - VIR_FREE(fullpath);
>> - VIR_FREE(xmlStr);
>> }
>> if (direrr < 0)
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -602,7 +591,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
>> ret = 0;
>> cleanup:
>> VIR_DIR_CLOSE(dir);
>> - VIR_FREE(chkDir);
>> virObjectUnlock(vm);
>> return ret;
>> }
>> @@ -660,12 +648,11 @@ qemuStateInitialize(bool privileged,
>> virStateInhibitCallback callback,
>> void *opaque)
>> {
>> - char *driverConf = NULL;
>> + VIR_AUTOFREE(char *) driverConf = NULL;
>> virQEMUDriverConfigPtr cfg;
>> uid_t run_uid = -1;
>> gid_t run_gid = -1;
>> - char *hugepagePath = NULL;
>> - char *memoryBackingPath = NULL;
>> + VIR_AUTOFREE(char *) memoryBackingPath = NULL;
>> bool autostart = true;
>> size_t i;
>>
>> @@ -706,7 +693,6 @@ qemuStateInitialize(bool privileged,
>>
>> if (virQEMUDriverConfigLoadFile(cfg, driverConf, privileged) < 0)
>> goto error;
>> - VIR_FREE(driverConf);
>>
>> if (virQEMUDriverConfigValidate(cfg) < 0)
>> goto error;
>> @@ -830,7 +816,7 @@ qemuStateInitialize(bool privileged,
>> goto error;
>>
>> if (privileged) {
>> - char *channeldir;
>> + VIR_AUTOFREE(char *) channeldir = NULL;
>>
>> if (chown(cfg->libDir, cfg->user, cfg->group) < 0) {
>> virReportSystemError(errno,
>> @@ -883,10 +869,8 @@ qemuStateInitialize(bool privileged,
>> _("unable to set ownership of '%s'
>> to %d:%d"),
>> channeldir, (int)cfg->user,
>> (int)cfg->group);
>> - VIR_FREE(channeldir);
>> goto error;
>> }
>> - VIR_FREE(channeldir);
>> if (chown(cfg->channelTargetDir, cfg->user, cfg->group) < 0) {
>> virReportSystemError(errno,
>> _("unable to set ownership of '%s'
>> to %d:%d"),
>> @@ -937,6 +921,8 @@ qemuStateInitialize(bool privileged,
>> * it, since we can't assume the root mount point has permissions
>> that
>> * will let our spawned QEMU instances use it. */
>> for (i = 0; i < cfg->nhugetlbfs; i++) {
>> + VIR_AUTOFREE(char *) hugepagePath = NULL;
>> +
>> hugepagePath = qemuGetBaseHugepagePath(&cfg->hugetlbfs[i]);
>>
>> if (!hugepagePath)
>> @@ -952,7 +938,6 @@ qemuStateInitialize(bool privileged,
>> virFileUpdatePerm(cfg->hugetlbfs[i].mnt_dir,
>> 0, S_IXGRP | S_IXOTH) < 0)
>> goto error;
>> - VIR_FREE(hugepagePath);
>> }
>>
>> if (qemuGetMemoryBackingBasePath(cfg, &memoryBackingPath) < 0)
>> @@ -969,7 +954,6 @@ qemuStateInitialize(bool privileged,
>> virFileUpdatePerm(memoryBackingPath,
>> 0, S_IXGRP | S_IXOTH) < 0)
>> goto error;
>> - VIR_FREE(memoryBackingPath);
>>
>> if (!(qemu_driver->closeCallbacks = virCloseCallbacksNew()))
>> goto error;
>> @@ -1038,9 +1022,6 @@ qemuStateInitialize(bool privileged,
>> return VIR_DRV_STATE_INIT_COMPLETE;
>>
>> error:
>> - VIR_FREE(driverConf);
>> - VIR_FREE(hugepagePath);
>> - VIR_FREE(memoryBackingPath);
>> qemuStateCleanup();
>> return VIR_DRV_STATE_INIT_ERROR;
>> }
>> @@ -1365,8 +1346,8 @@ static int
>> qemuGetSchedInfo(unsigned long long *cpuWait,
>> pid_t pid, pid_t tid)
>> {
>> - char *proc = NULL;
>> - char *data = NULL;
>> + VIR_AUTOFREE(char *) proc = NULL;
>> + VIR_AUTOFREE(char *) data = NULL;
>> char **lines = NULL;
>> size_t i;
>> int ret = -1;
>> @@ -1430,8 +1411,6 @@ qemuGetSchedInfo(unsigned long long *cpuWait,
>> ret = 0;
>>
>> cleanup:
>> - VIR_FREE(data);
>> - VIR_FREE(proc);
>> virStringListFree(lines);
>> return ret;
>> }
>> @@ -1441,7 +1420,7 @@ static int
>> qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long
>> *vm_rss,
>> pid_t pid, int tid)
>> {
>> - char *proc;
>> + VIR_AUTOFREE(char *) proc = NULL;
>> FILE *pidinfo;
>> unsigned long long usertime = 0, systime = 0;
>> long rss = 0;
>> @@ -1458,7 +1437,6 @@ qemuGetProcessInfo(unsigned long long *cpuTime,
>> int *lastCpu, long *vm_rss,
>> return -1;
>>
>> pidinfo = fopen(proc, "r");
>> - VIR_FREE(proc);
>>
>> /* See 'man proc' for information about what all these fields
>> are. We're
>> * only interested in a very few of them */
>> @@ -2910,9 +2888,8 @@ virQEMUSaveDataWrite(virQEMUSaveDataPtr data,
>> size_t len;
>> size_t xml_len;
>> size_t cookie_len = 0;
>> - int ret = -1;
>> size_t zerosLen = 0;
>> - char *zeros = NULL;
>> + VIR_AUTOFREE(char *) zeros = NULL;
>>
>> xml_len = strlen(data->xml) + 1;
>> if (data->cookie)
>> @@ -2924,12 +2901,12 @@ virQEMUSaveDataWrite(virQEMUSaveDataPtr data,
>> if (len > header->data_len) {
>> virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> _("new xml too large to fit in file"));
>> - goto cleanup;
>> + return -1;
>> }
>>
>> zerosLen = header->data_len - len;
>> if (VIR_ALLOC_N(zeros, zerosLen) < 0)
>> - goto cleanup;
>> + return -1;
>> } else {
>> header->data_len = len;
>> }
>> @@ -2941,14 +2918,14 @@ virQEMUSaveDataWrite(virQEMUSaveDataPtr data,
>> virReportSystemError(errno,
>> _("failed to write header to domain save
>> file '%s'"),
>> path);
>> - goto cleanup;
>> + return -1;
>> }
>>
>> if (safewrite(fd, data->xml, xml_len) != xml_len) {
>> virReportSystemError(errno,
>> _("failed to write domain xml to '%s'"),
>> path);
>> - goto cleanup;
>> + return -1;
>> }
>>
>> if (data->cookie &&
>> @@ -2956,21 +2933,17 @@ virQEMUSaveDataWrite(virQEMUSaveDataPtr data,
>> virReportSystemError(errno,
>> _("failed to write cookie to '%s'"),
>> path);
>> - goto cleanup;
>> + return -1;
>> }
>>
>> if (safewrite(fd, zeros, zerosLen) != zerosLen) {
>> virReportSystemError(errno,
>> _("failed to write padding to '%s'"),
>> path);
>> - goto cleanup;
>> + return -1;
>> }
>>
>> - ret = 0;
>> -
>> - cleanup:
>> - VIR_FREE(zeros);
>> - return ret;
>> + return 0;
>> }
>>
>>
>> @@ -3300,7 +3273,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
>> int compressed, const char *compressedpath,
>> const char *xmlin, unsigned int flags)
>> {
>> - char *xml = NULL;
>> + VIR_AUTOFREE(char *) xml = NULL;
>> bool was_running = false;
>> int ret = -1;
>> virObjectEventPtr event = NULL;
>> @@ -3381,7 +3354,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
>> if (!(data = virQEMUSaveDataNew(xml, cookie, was_running,
>> compressed,
>> driver->xmlopt)))
>> goto endjob;
>> - xml = NULL;
>>
>> ret = qemuDomainSaveMemory(driver, vm, path, data, compressedpath,
>> flags, QEMU_ASYNC_JOB_SAVE);
>> @@ -3416,7 +3388,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
>> qemuDomainRemoveInactiveJob(driver, vm);
>>
>> cleanup:
>> - VIR_FREE(xml);
>> virQEMUSaveDataFree(data);
>> virObjectEventStateQueue(driver->domainEventState, event);
>> return ret;
>> @@ -3504,7 +3475,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const
>> char *path, const char *dxml,
>> {
>> virQEMUDriverPtr driver = dom->conn->privateData;
>> int compressed;
>> - char *compressedpath = NULL;
>> + VIR_AUTOFREE(char *) compressedpath = NULL;
>> int ret = -1;
>> virDomainObjPtr vm = NULL;
>> VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = NULL;
>> @@ -3533,7 +3504,6 @@ qemuDomainSaveFlags(virDomainPtr dom, const
>> char *path, const char *dxml,
>>
>> cleanup:
>> virDomainObjEndAPI(&vm);
>> - VIR_FREE(compressedpath);
>> return ret;
>> }
>>
>> @@ -3561,9 +3531,9 @@ qemuDomainManagedSave(virDomainPtr dom,
>> unsigned int flags)
>> virQEMUDriverPtr driver = dom->conn->privateData;
>> VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = NULL;
>> int compressed;
>> - char *compressedpath = NULL;
>> + VIR_AUTOFREE(char *) compressedpath = NULL;
>> virDomainObjPtr vm;
>> - char *name = NULL;
>> + VIR_AUTOFREE(char *) name = NULL;
>> int ret = -1;
>>
>> virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE |
>> @@ -3603,8 +3573,6 @@ qemuDomainManagedSave(virDomainPtr dom,
>> unsigned int flags)
>>
>> cleanup:
>> virDomainObjEndAPI(&vm);
>> - VIR_FREE(name);
>> - VIR_FREE(compressedpath);
>>
>> return ret;
>> }
>> @@ -3614,7 +3582,7 @@ qemuDomainManagedSaveLoad(virDomainObjPtr vm,
>> void *opaque)
>> {
>> virQEMUDriverPtr driver = opaque;
>> - char *name;
>> + VIR_AUTOFREE(char *) name = NULL;
>> int ret = -1;
>>
>> virObjectLock(vm);
>> @@ -3627,7 +3595,6 @@ qemuDomainManagedSaveLoad(virDomainObjPtr vm,
>> ret = 0;
>> cleanup:
>> virObjectUnlock(vm);
>> - VIR_FREE(name);
>> return ret;
>> }
>>
>> @@ -3659,7 +3626,7 @@ qemuDomainManagedSaveRemove(virDomainPtr dom,
>> unsigned int flags)
>> virQEMUDriverPtr driver = dom->conn->privateData;
>> virDomainObjPtr vm;
>> int ret = -1;
>> - char *name = NULL;
>> + VIR_AUTOFREE(char *) name = NULL;
>>
>> virCheckFlags(0, -1);
>>
>> @@ -3683,7 +3650,6 @@ qemuDomainManagedSaveRemove(virDomainPtr dom,
>> unsigned int flags)
>> ret = 0;
>>
>> cleanup:
>> - VIR_FREE(name);
>> virDomainObjEndAPI(&vm);
>> return ret;
>> }
>> @@ -3803,7 +3769,7 @@ doCoreDump(virQEMUDriverPtr driver,
>> unsigned int flags = VIR_FILE_WRAPPER_NON_BLOCKING;
>> const char *memory_dump_format = NULL;
>> VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg =
>> virQEMUDriverGetConfig(driver);
>> - char *compressedpath = NULL;
>> + VIR_AUTOFREE(char *) compressedpath = NULL;
>>
>> /* We reuse "save" flag for "dump" here. Then, we can support the
>> same
>> * format in "save" and "dump". This path doesn't need the
>> compression
>> @@ -3883,7 +3849,6 @@ doCoreDump(virQEMUDriverPtr driver,
>> virFileWrapperFdFree(wrapperFd);
>> if (ret != 0)
>> unlink(path);
>> - VIR_FREE(compressedpath);
>> return ret;
>> }
>>
>> @@ -4009,7 +3974,7 @@ qemuDomainScreenshot(virDomainPtr dom,
>> virQEMUDriverPtr driver = dom->conn->privateData;
>> virDomainObjPtr vm;
>> qemuDomainObjPrivatePtr priv;
>> - char *tmp = NULL;
>> + VIR_AUTOFREE(char *) tmp = NULL;
>> int tmp_fd = -1;
>> size_t i;
>> const char *videoAlias = NULL;
>> @@ -4101,7 +4066,6 @@ qemuDomainScreenshot(virDomainPtr dom,
>> VIR_FORCE_CLOSE(tmp_fd);
>> if (unlink_tmp)
>> unlink(tmp);
>> - VIR_FREE(tmp);
>>
>> qemuDomainObjEndJob(driver, vm);
>>
>> @@ -4115,7 +4079,7 @@ getAutoDumpPath(virQEMUDriverPtr driver,
>> virDomainObjPtr vm)
>> {
>> char *dumpfile = NULL;
>> - char *domname = virDomainDefGetShortName(vm->def);
>> + VIR_AUTOFREE(char *domname) = virDomainDefGetShortName(vm->def);
>> char timestr[100];
>> struct tm time_info;
>> time_t curtime = time(NULL);
>> @@ -4134,7 +4098,6 @@ getAutoDumpPath(virQEMUDriverPtr driver,
>> domname,
>> timestr));
>>
>> - VIR_FREE(domname);
>> return dumpfile;
>> }
>>
>> --
>> 2.21.0
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list