Re: [PATCH v4 1/4] qemu_process.c: use g_autofree

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> ---
>  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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux