Re: [libvirt] [PATCH v3 06/10] qemu_driver: use VIR_AUTOFREE() with strings 1/3

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)

* 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. 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(a)gmail.com&gt; > --- > 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(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list