Modify virCgroupFree function signature to take a value of type
virCgroupPtr instead of virCgroupPtr * as the parameter.

Change the argument type in all calls to virCgroupFree function
from virCgroupPtr * to virCgroupPtr.

Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
---
 src/libvirt-lxc.c        |  4 ++--
 src/lxc/lxc_cgroup.c     |  4 ++--
 src/lxc/lxc_container.c  |  2 +-
 src/lxc/lxc_controller.c |  2 +-
 src/lxc/lxc_domain.c     |  2 +-
 src/lxc/lxc_process.c    | 10 ++++-----
 src/qemu/qemu_cgroup.c   | 16 +++++++-------
 src/qemu/qemu_domain.c   |  2 +-
 src/qemu/qemu_driver.c   | 34 ++++++++++++++---------------
 src/qemu/qemu_process.c  |  2 +-
 src/util/vircgroup.c     | 56 ++++++++++++++++++++++++------------------------
 src/util/vircgroup.h     |  2 +-
 tests/vircgrouptest.c    | 42 ++++++++++++++++++------------------
 13 files changed, 88 insertions(+), 90 deletions(-)

diff --git a/src/libvirt-lxc.c b/src/libvirt-lxc.c
index c9f2146..12be893 100644
--- a/src/libvirt-lxc.c
+++ b/src/libvirt-lxc.c
@@ -309,12 +309,12 @@ int virDomainLxcEnterCGroup(virDomainPtr domain,
     if (virCgroupAddTask(cgroup, getpid()) < 0)
         goto error;
 
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
 
     return 0;
 
  error:
     virDispatchError(NULL);
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     return -1;
 }
diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index 8e937ec..873c843 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -306,7 +306,7 @@ int virLXCCgroupGetMeminfo(virLXCMeminfoPtr meminfo)
 
     ret = 0;
  cleanup:
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     return ret;
 }
 
@@ -515,7 +515,7 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def,
                               def->idmap.uidmap[0].target,
                               def->idmap.gidmap[0].target,
                               (1 << VIR_CGROUP_CONTROLLER_SYSTEMD)) < 0) {
-            virCgroupFree(&cgroup);
+            virCgroupFree(cgroup);
             cgroup = NULL;
             goto cleanup;
         }
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 3a1b2d6..407214f 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -1815,7 +1815,7 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef,
 
  cleanup:
     VIR_FREE(stateDir);
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     VIR_FREE(sec_mount_options);
     return ret;
 }
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 4e84391..7be45f8 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -296,7 +296,7 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl)
     VIR_FREE(ctrl->nbdpids);
 
     VIR_FREE(ctrl->nsFDs);
-    virCgroupFree(&ctrl->cgroup);
+    virCgroupFree(ctrl->cgroup);
 
     /* This must always be the last thing to be closed */
     VIR_FORCE_CLOSE(ctrl->handshakeFd);
diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
index b197f9d..eb0071d 100644
--- a/src/lxc/lxc_domain.c
+++ b/src/lxc/lxc_domain.c
@@ -172,7 +172,7 @@ virLXCDomainObjPrivateFree(void *data)
 {
     virLXCDomainObjPrivatePtr priv = data;
 
-    virCgroupFree(&priv->cgroup);
+    virCgroupFree(priv->cgroup);
     virLXCDomainObjFreeJob(priv);
     VIR_FREE(priv);
 }
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 14502e1..8534051 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -220,7 +220,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver,
 
     if (priv->cgroup) {
         virCgroupRemove(priv->cgroup);
-        virCgroupFree(&priv->cgroup);
+        virCgroupFree(priv->cgroup);
     }
 
     /* Get machined to terminate the machine as it may not have cleaned it
@@ -1203,26 +1203,26 @@ int virLXCProcessStart(virConnectPtr conn,
 
     if (!virCgroupHasController(selfcgroup,
                                 VIR_CGROUP_CONTROLLER_CPUACCT)) {
-        virCgroupFree(&selfcgroup);
+        virCgroupFree(selfcgroup);
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Unable to find 'cpuacct' cgroups controller mount"));
         return -1;
     }
     if (!virCgroupHasController(selfcgroup,
                                 VIR_CGROUP_CONTROLLER_DEVICES)) {
-        virCgroupFree(&selfcgroup);
+        virCgroupFree(selfcgroup);
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Unable to find 'devices' cgroups controller mount"));
         return -1;
     }
     if (!virCgroupHasController(selfcgroup,
                                 VIR_CGROUP_CONTROLLER_MEMORY)) {
-        virCgroupFree(&selfcgroup);
+        virCgroupFree(selfcgroup);
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Unable to find 'memory' cgroups controller mount"));
         return -1;
     }
-    virCgroupFree(&selfcgroup);
+    virCgroupFree(selfcgroup);
 
     if (vm->def->nconsoles == 0) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 43e17d7..8a00ffc 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -841,7 +841,7 @@ qemuSetupCpusetMems(virDomainObjPtr vm)
     ret = 0;
  cleanup:
     VIR_FREE(mem_mask);
-    virCgroupFree(&cgroup_temp);
+    virCgroupFree(cgroup_temp);
     return ret;
 }
 
@@ -920,7 +920,7 @@ qemuInitCgroup(virDomainObjPtr vm,
     if (!virCgroupAvailable())
         goto done;
 
-    virCgroupFree(&priv->cgroup);
+    virCgroupFree(priv->cgroup);
 
     if (!vm->def->resource) {
         virDomainResourceDefPtr res;
@@ -1008,7 +1008,7 @@ qemuRestoreCgroupState(virDomainObjPtr vm)
             goto cleanup;
 
         VIR_FREE(nodeset);
-        virCgroupFree(&cgroup_temp);
+        virCgroupFree(cgroup_temp);
     }
 
     for (i = 0; i < vm->def->niothreadids; i++) {
@@ -1021,7 +1021,7 @@ qemuRestoreCgroupState(virDomainObjPtr vm)
             goto cleanup;
 
         VIR_FREE(nodeset);
-        virCgroupFree(&cgroup_temp);
+        virCgroupFree(cgroup_temp);
     }
 
     if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0,
@@ -1035,7 +1035,7 @@ qemuRestoreCgroupState(virDomainObjPtr vm)
     VIR_FREE(mem_mask);
     VIR_FREE(nodeset);
     virBitmapFree(all_nodes);
-    virCgroupFree(&cgroup_temp);
+    virCgroupFree(cgroup_temp);
     return;
 
  error:
@@ -1057,7 +1057,7 @@ qemuConnectCgroup(virDomainObjPtr vm)
     if (!virCgroupAvailable())
         goto done;
 
-    virCgroupFree(&priv->cgroup);
+    virCgroupFree(priv->cgroup);
 
     if (virCgroupNewDetectMachine(vm->def->name,
                                   "qemu",
@@ -1203,7 +1203,7 @@ qemuSetupCgroupForExtDevices(virDomainObjPtr vm,
     ret = qemuExtDevicesSetupCgroup(driver, vm->def, cgroup_temp);
 
  cleanup:
-    virCgroupFree(&cgroup_temp);
+    virCgroupFree(cgroup_temp);
 
     return ret;
 }
@@ -1281,7 +1281,7 @@ qemuCgroupEmulatorAllNodesDataFree(qemuCgroupEmulatorAllNodesDataPtr data)
     if (!data)
         return;
 
-    virCgroupFree(&data->emulatorCgroup);
+    virCgroupFree(data->emulatorCgroup);
     VIR_FREE(data->emulatorMemMask);
     VIR_FREE(data);
 }
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9188302..26eeb8f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1920,7 +1920,7 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv)
     virStringListFree(priv->qemuDevices);
     priv->qemuDevices = NULL;
 
-    virCgroupFree(&priv->cgroup);
+    virCgroupFree(priv->cgroup);
 
     virPerfFree(priv->perf);
     priv->perf = NULL;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8cdb04e..606fd72 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5075,7 +5075,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
 
  cleanup:
     virBitmapFree(tmpmap);
-    virCgroupFree(&cgroup_vcpu);
+    virCgroupFree(cgroup_vcpu);
     VIR_FREE(str);
     virObjectEventStateQueue(driver->domainEventState, event);
     return ret;
@@ -5312,8 +5312,7 @@ qemuDomainPinEmulator(virDomainPtr dom,
     qemuDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (cgroup_emulator)
-        virCgroupFree(&cgroup_emulator);
+    virCgroupFree(cgroup_emulator);
     virObjectEventStateQueue(driver->domainEventState, event);
     VIR_FREE(str);
     virBitmapFree(pcpumap);
@@ -5794,8 +5793,7 @@ qemuDomainPinIOThread(virDomainPtr dom,
     qemuDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (cgroup_iothread)
-        virCgroupFree(&cgroup_iothread);
+    virCgroupFree(cgroup_iothread);
     virObjectEventStateQueue(driver->domainEventState, event);
     VIR_FREE(str);
     virBitmapFree(pcpumap);
@@ -9855,7 +9853,7 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
                            false, &cgroup_temp) < 0 ||
         virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0)
         goto cleanup;
-    virCgroupFree(&cgroup_temp);
+    virCgroupFree(cgroup_temp);
 
     for (i = 0; i < virDomainDefGetVcpusMax(vm->def); i++) {
         virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i);
@@ -9867,7 +9865,7 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
                                false, &cgroup_temp) < 0 ||
             virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0)
             goto cleanup;
-        virCgroupFree(&cgroup_temp);
+        virCgroupFree(cgroup_temp);
     }
 
     for (i = 0; i < vm->def->niothreadids; i++) {
@@ -9876,13 +9874,13 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
                                false, &cgroup_temp) < 0 ||
             virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0)
             goto cleanup;
-        virCgroupFree(&cgroup_temp);
+        virCgroupFree(cgroup_temp);
     }
 
     ret = 0;
  cleanup:
     VIR_FREE(nodeset_str);
-    virCgroupFree(&cgroup_temp);
+    virCgroupFree(cgroup_temp);
 
     return ret;
 }
@@ -10304,13 +10302,13 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup,
         if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0)
             goto cleanup;
 
-        virCgroupFree(&cgroup_vcpu);
+        virCgroupFree(cgroup_vcpu);
     }
 
     return 0;
 
  cleanup:
-    virCgroupFree(&cgroup_vcpu);
+    virCgroupFree(cgroup_vcpu);
     return -1;
 }
 
@@ -10331,11 +10329,11 @@ qemuSetEmulatorBandwidthLive(virCgroupPtr cgroup,
     if (qemuSetupCgroupVcpuBW(cgroup_emulator, period, quota) < 0)
         goto cleanup;
 
-    virCgroupFree(&cgroup_emulator);
+    virCgroupFree(cgroup_emulator);
     return 0;
 
  cleanup:
-    virCgroupFree(&cgroup_emulator);
+    virCgroupFree(cgroup_emulator);
     return -1;
 }
 
@@ -10362,13 +10360,13 @@ qemuSetIOThreadsBWLive(virDomainObjPtr vm, virCgroupPtr cgroup,
         if (qemuSetupCgroupVcpuBW(cgroup_iothread, period, quota) < 0)
             goto cleanup;
 
-        virCgroupFree(&cgroup_iothread);
+        virCgroupFree(cgroup_iothread);
     }
 
     return 0;
 
  cleanup:
-    virCgroupFree(&cgroup_iothread);
+    virCgroupFree(cgroup_iothread);
     return -1;
 }
 
@@ -10754,7 +10752,7 @@ qemuGetVcpusBWLive(virDomainObjPtr vm,
     ret = 0;
 
  cleanup:
-    virCgroupFree(&cgroup_vcpu);
+    virCgroupFree(cgroup_vcpu);
     return ret;
 }
 
@@ -10777,7 +10775,7 @@ qemuGetEmulatorBandwidthLive(virCgroupPtr cgroup,
     ret = 0;
 
  cleanup:
-    virCgroupFree(&cgroup_emulator);
+    virCgroupFree(cgroup_emulator);
     return ret;
 }
 
@@ -10813,7 +10811,7 @@ qemuGetIOThreadsBWLive(virDomainObjPtr vm,
     ret = 0;
 
  cleanup:
-    virCgroupFree(&cgroup_iothread);
+    virCgroupFree(cgroup_iothread);
     return ret;
 }
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a4b1f97..b3abaae 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2532,7 +2532,7 @@ qemuProcessSetupPid(virDomainObjPtr vm,
     if (cgroup) {
         if (ret < 0)
             virCgroupRemove(cgroup);
-        virCgroupFree(&cgroup);
+        virCgroupFree(cgroup);
     }
 
     return ret;
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index e810a3d..140b016 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1178,7 +1178,7 @@ virCgroupNew(pid_t pid,
     return 0;
 
  error:
-    virCgroupFree(group);
+    virCgroupFree(*group);
     *group = NULL;
 
     return -1;
@@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path,
     ret = 0;
  cleanup:
     if (ret != 0)
-        virCgroupFree(group);
-    virCgroupFree(&parent);
+        virCgroupFree(*group);
+    virCgroupFree(parent);
     VIR_FREE(parentPath);
     VIR_FREE(newPath);
     return ret;
@@ -1447,7 +1447,7 @@ virCgroupNewDomainPartition(virCgroupPtr partition,
     if (virCgroupMakeGroup(partition, *group, create,
                            VIR_CGROUP_MEM_HIERACHY) < 0) {
         virCgroupRemove(*group);
-        virCgroupFree(group);
+        virCgroupFree(*group);
         goto cleanup;
     }
 
@@ -1509,7 +1509,7 @@ virCgroupNewThread(virCgroupPtr domain,
 
     if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE) < 0) {
         virCgroupRemove(*group);
-        virCgroupFree(group);
+        virCgroupFree(*group);
         goto cleanup;
     }
 
@@ -1550,7 +1550,7 @@ virCgroupNewDetectMachine(const char *name,
                                        true, machinename)) {
         VIR_DEBUG("Failed to validate machine name for '%s' driver '%s'",
                   name, drivername);
-        virCgroupFree(group);
+        virCgroupFree(*group);
         return 0;
     }
 
@@ -1603,7 +1603,7 @@ virCgroupNewMachineSystemd(const char *name,
 
     path = init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement;
     init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement = NULL;
-    virCgroupFree(&init);
+    virCgroupFree(init);
 
     if (!path || STREQ(path, "/") || path[0] != '/') {
         VIR_DEBUG("Systemd didn't setup its controller");
@@ -1635,13 +1635,13 @@ virCgroupNewMachineSystemd(const char *name,
             goto cleanup;
 
         if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_NONE) < 0) {
-            virCgroupFree(&tmp);
+            virCgroupFree(tmp);
             goto cleanup;
         }
         if (t) {
             *t = '/';
             offset = t;
-            virCgroupFree(&parent);
+            virCgroupFree(parent);
             parent = tmp;
         } else {
             *group = tmp;
@@ -1652,7 +1652,7 @@ virCgroupNewMachineSystemd(const char *name,
     if (virCgroupAddTask(*group, pidleader) < 0) {
         virErrorPtr saved = virSaveLastError();
         virCgroupRemove(*group);
-        virCgroupFree(group);
+        virCgroupFree(*group);
         if (saved) {
             virSetError(saved);
             virFreeError(saved);
@@ -1661,7 +1661,7 @@ virCgroupNewMachineSystemd(const char *name,
 
     ret = 0;
  cleanup:
-    virCgroupFree(&parent);
+    virCgroupFree(parent);
     VIR_FREE(path);
     return ret;
 }
@@ -1708,7 +1708,7 @@ virCgroupNewMachineManual(const char *name,
     if (virCgroupAddTask(*group, pidleader) < 0) {
         virErrorPtr saved = virSaveLastError();
         virCgroupRemove(*group);
-        virCgroupFree(group);
+        virCgroupFree(*group);
         if (saved) {
             virSetError(saved);
             virFreeError(saved);
@@ -1719,7 +1719,7 @@ virCgroupNewMachineManual(const char *name,
     ret = 0;
 
  cleanup:
-    virCgroupFree(&parent);
+    virCgroupFree(parent);
     return ret;
 }
 
@@ -1786,21 +1786,21 @@ virCgroupNewIgnoreError(void)
  * @group: The group structure to free
  */
 void
-virCgroupFree(virCgroupPtr *group)
+virCgroupFree(virCgroupPtr group)
 {
     size_t i;
 
-    if (*group == NULL)
+    if (!group)
         return;
 
     for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
-        VIR_FREE((*group)->controllers[i].mountPoint);
-        VIR_FREE((*group)->controllers[i].linkPoint);
-        VIR_FREE((*group)->controllers[i].placement);
+        VIR_FREE(group->controllers[i].mountPoint);
+        VIR_FREE(group->controllers[i].linkPoint);
+        VIR_FREE(group->controllers[i].placement);
     }
 
-    VIR_FREE((*group)->path);
-    VIR_FREE(*group);
+    VIR_FREE(group->path);
+    VIR_FREE(group);
 }
 
 
@@ -2514,7 +2514,7 @@ virCgroupMemoryOnceInit(void)
                                       "memory.limit_in_bytes",
                                       &mem_unlimited));
  cleanup:
-    virCgroupFree(&group);
+    virCgroupFree(group);
     virCgroupMemoryUnlimitedKB = mem_unlimited >> 10;
 }
 
@@ -3158,13 +3158,13 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group,
             sum_cpu_time[j] += tmp;
         }
 
-        virCgroupFree(&group_vcpu);
+        virCgroupFree(group_vcpu);
         VIR_FREE(buf);
     }
 
     ret = 0;
  cleanup:
-    virCgroupFree(&group_vcpu);
+    virCgroupFree(group_vcpu);
     VIR_FREE(buf);
     return ret;
 }
@@ -3722,7 +3722,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
         if (dormdir)
             virCgroupRemove(subgroup);
 
-        virCgroupFree(&subgroup);
+        virCgroupFree(subgroup);
     }
     if (direrr < 0)
         goto cleanup;
@@ -3731,7 +3731,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
     ret = killedAny ? 1 : 0;
 
  cleanup:
-    virCgroupFree(&subgroup);
+    virCgroupFree(subgroup);
     VIR_FREE(keypath);
     VIR_DIR_CLOSE(dp);
     return ret;
@@ -4118,7 +4118,7 @@ virCgroupControllerAvailable(int controller)
         return ret;
 
     ret = virCgroupHasController(cgroup, controller);
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     return ret;
 }
 
@@ -4250,7 +4250,7 @@ virCgroupNewIgnoreError(void)
 
 
 void
-virCgroupFree(virCgroupPtr *group ATTRIBUTE_UNUSED)
+virCgroupFree(virCgroupPtr group ATTRIBUTE_UNUSED)
 {
     virReportSystemError(ENXIO, "%s",
                          _("Control groups not supported on this platform"));
@@ -4915,7 +4915,7 @@ virCgroupDelThread(virCgroupPtr cgroup,
 
         /* Remove the offlined cgroup */
         virCgroupRemove(new_cgroup);
-        virCgroupFree(&new_cgroup);
+        virCgroupFree(new_cgroup);
     }
 
     return 0;
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index d833927..e4ffd57 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -122,7 +122,7 @@ int virCgroupTerminateMachine(const char *name)
 
 bool virCgroupNewIgnoreError(void);
 
-void virCgroupFree(virCgroupPtr *group);
+void virCgroupFree(virCgroupPtr group);
 
 bool virCgroupHasController(virCgroupPtr cgroup, int controller);
 int virCgroupPathOfController(virCgroupPtr group,
diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
index be50f3e..e5190e3 100644
--- a/tests/vircgrouptest.c
+++ b/tests/vircgrouptest.c
@@ -198,7 +198,7 @@ testCgroupDetectMounts(const void *args)
  cleanup:
     VIR_FREE(mounts);
     VIR_FREE(parsed);
-    virCgroupFree(&group);
+    virCgroupFree(group);
     virBufferFreeAndReset(&buf);
     return result;
 }
@@ -227,7 +227,7 @@ static int testCgroupNewForSelf(const void *args ATTRIBUTE_UNUSED)
     ret = validateCgroup(cgroup, "", mountsFull, links, placement);
 
  cleanup:
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     return ret;
 }
 
@@ -304,7 +304,7 @@ static int testCgroupNewForPartition(const void *args ATTRIBUTE_UNUSED)
         goto cleanup;
     }
     ret = validateCgroup(cgroup, "/virtualmachines.partition", mountsSmall, links, placementSmall);
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
 
     if ((rv = virCgroupNewPartition("/virtualmachines", true, -1, &cgroup)) != 0) {
         fprintf(stderr, "Cannot create /virtualmachines cgroup: %d\n", -rv);
@@ -313,7 +313,7 @@ static int testCgroupNewForPartition(const void *args ATTRIBUTE_UNUSED)
     ret = validateCgroup(cgroup, "/virtualmachines.partition", mountsFull, links, placementFull);
 
  cleanup:
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     return ret;
 }
 
@@ -353,7 +353,7 @@ static int testCgroupNewForPartitionNested(const void *args ATTRIBUTE_UNUSED)
     }
 
     /* Should now work */
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     if ((rv = virCgroupNewPartition("/deployment/production", true, -1, &cgroup)) != 0) {
         fprintf(stderr, "Failed to create /deployment/production cgroup: %d\n", -rv);
         goto cleanup;
@@ -363,7 +363,7 @@ static int testCgroupNewForPartitionNested(const void *args ATTRIBUTE_UNUSED)
                          mountsFull, links, placementFull);
 
  cleanup:
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     return ret;
 }
 
@@ -402,14 +402,14 @@ static int testCgroupNewForPartitionNestedDeep(const void *args ATTRIBUTE_UNUSED
         goto cleanup;
     }
 
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     if ((rv = virCgroupNewPartition("/user/berrange.user", true, -1, &cgroup)) != 0) {
         fprintf(stderr, "Failed to create /user/berrange.user cgroup: %d\n", -rv);
         goto cleanup;
     }
 
     /* Should now work */
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     if ((rv = virCgroupNewPartition("/user/berrange.user/production", true, -1, &cgroup)) != 0) {
         fprintf(stderr, "Failed to create /user/berrange.user/production cgroup: %d\n", -rv);
         goto cleanup;
@@ -419,7 +419,7 @@ static int testCgroupNewForPartitionNestedDeep(const void *args ATTRIBUTE_UNUSED
                          mountsFull, links, placementFull);
 
  cleanup:
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     return ret;
 }
 
@@ -455,8 +455,8 @@ static int testCgroupNewForPartitionDomain(const void *args ATTRIBUTE_UNUSED)
     ret = validateCgroup(domaincgroup, "/production.partition/foo.libvirt-lxc", mountsFull, links, placement);
 
  cleanup:
-    virCgroupFree(&partitioncgroup);
-    virCgroupFree(&domaincgroup);
+    virCgroupFree(partitioncgroup);
+    virCgroupFree(domaincgroup);
     return ret;
 }
 
@@ -506,10 +506,10 @@ static int testCgroupNewForPartitionDomainEscaped(const void *args ATTRIBUTE_UNU
     ret = validateCgroup(domaincgroup, "/_cgroup.evil/net_cls.evil/__evil.evil/_cpu.foo.libvirt-lxc", mountsFull, links, placement);
 
  cleanup:
-    virCgroupFree(&partitioncgroup3);
-    virCgroupFree(&partitioncgroup2);
-    virCgroupFree(&partitioncgroup1);
-    virCgroupFree(&domaincgroup);
+    virCgroupFree(partitioncgroup3);
+    virCgroupFree(partitioncgroup2);
+    virCgroupFree(partitioncgroup1);
+    virCgroupFree(domaincgroup);
     return ret;
 }
 
@@ -535,7 +535,7 @@ static int testCgroupNewForSelfAllInOne(const void *args ATTRIBUTE_UNUSED)
     ret = validateCgroup(cgroup, "", mountsAllInOne, linksAllInOne, placement);
 
  cleanup:
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     return ret;
 }
 
@@ -563,7 +563,7 @@ static int testCgroupNewForSelfLogind(const void *args ATTRIBUTE_UNUSED)
     ret = validateCgroup(cgroup, "", mountsLogind, linksLogind, placement);
 
  cleanup:
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     return ret;
 }
 
@@ -690,7 +690,7 @@ static int testCgroupGetPercpuStats(const void *args ATTRIBUTE_UNUSED)
     ret = 0;
 
  cleanup:
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     VIR_FREE(params);
     return ret;
 }
@@ -723,7 +723,7 @@ static int testCgroupGetMemoryUsage(const void *args ATTRIBUTE_UNUSED)
     ret = 0;
 
  cleanup:
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     return ret;
 }
 
@@ -773,7 +773,7 @@ static int testCgroupGetBlkioIoServiced(const void *args ATTRIBUTE_UNUSED)
     ret = 0;
 
  cleanup:
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     return ret;
 }
 
@@ -846,7 +846,7 @@ static int testCgroupGetBlkioIoDeviceServiced(const void *args ATTRIBUTE_UNUSED)
     ret = 0;
 
  cleanup:
-    virCgroupFree(&cgroup);
+    virCgroupFree(cgroup);
     return ret;
 }
 
-- 
1.8.3.1

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

On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
> Modify virCgroupFree function signature to take a value of type
> virCgroupPtr instead of virCgroupPtr * as the parameter.
>
> Change the argument type in all calls to virCgroupFree function
> from virCgroupPtr * to virCgroupPtr.

^This sentence doesn't add any useful information. Instead, the commit message
should add information about why we're performing this change, i.e. in order to
enable usage of VIR_AUTOPTR with cgroup module or something alike.
Also, this patch is oddly placed, IMHO it should come before patch 8, where the
other work on cgroup module is done.

With that:
Reviewed-by: Erik Skultety <eskultet@redhat.com>

...

> @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path,
>      ret = 0;
>   cleanup:
>      if (ret != 0)
> -        virCgroupFree(group);
> -    virCgroupFree(&parent);
> +        virCgroupFree(*group);
> +    virCgroupFree(parent);

Since you're already touching the code, I'd appreciate another "adjustment"
patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, IOW
where we're passing a reference to a pointer in order to change the original
pointer, we should use a VIR_STEAL_PTR just before the cleanup section, so that
we don't need an if (ret != 0) or if (ret < 0) check only to conditionally do
some cleanup. Feel free to let me know if none of what I just wrote is clear.

Erik

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

On Mon, 23 Jul 2018 at 16:29, Erik Skultety <eskultet@redhat.com> wrote:
>
> On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
> > Modify virCgroupFree function signature to take a value of type
> > virCgroupPtr instead of virCgroupPtr * as the parameter.
> >
> > Change the argument type in all calls to virCgroupFree function
> > from virCgroupPtr * to virCgroupPtr.
>
> ^This sentence doesn't add any useful information. Instead, the commit message
> should add information about why we're performing this change, i.e. in order to
> enable usage of VIR_AUTOPTR with cgroup module or something alike.
> Also, this patch is oddly placed, IMHO it should come before patch 8, where the
> other work on cgroup module is done.
>
> With that:
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
>
> ...
>
> > @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path,
> >      ret = 0;
> >   cleanup:
> >      if (ret != 0)
> > -        virCgroupFree(group);
> > -    virCgroupFree(&parent);
> > +        virCgroupFree(*group);
> > +    virCgroupFree(parent);
>
> Since you're already touching the code, I'd appreciate another "adjustment"
> patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, IOW
> where we're passing a reference to a pointer in order to change the original
> pointer, we should use a VIR_STEAL_PTR just before the cleanup section, so that
> we don't need an if (ret != 0) or if (ret < 0) check only to conditionally do
> some cleanup. Feel free to let me know if none of what I just wrote is clear.

I am assuming that you are referring to `group` variable. If so, then I cannot
apply cleanup attribute to function parameters and `group` is one of them.

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

On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote:
> On Mon, 23 Jul 2018 at 16:29, Erik Skultety <eskultet@redhat.com> wrote:
> >
> > On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
> > > Modify virCgroupFree function signature to take a value of type
> > > virCgroupPtr instead of virCgroupPtr * as the parameter.
> > >
> > > Change the argument type in all calls to virCgroupFree function
> > > from virCgroupPtr * to virCgroupPtr.
> >
> > ^This sentence doesn't add any useful information. Instead, the commit message
> > should add information about why we're performing this change, i.e. in order to
> > enable usage of VIR_AUTOPTR with cgroup module or something alike.
> > Also, this patch is oddly placed, IMHO it should come before patch 8, where the
> > other work on cgroup module is done.
> >
> > With that:
> > Reviewed-by: Erik Skultety <eskultet@redhat.com>
> >
> > ...
> >
> > > @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path,
> > >      ret = 0;
> > >   cleanup:
> > >      if (ret != 0)
> > > -        virCgroupFree(group);
> > > -    virCgroupFree(&parent);
> > > +        virCgroupFree(*group);
> > > +    virCgroupFree(parent);
> >
> > Since you're already touching the code, I'd appreciate another "adjustment"
> > patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, IOW
> > where we're passing a reference to a pointer in order to change the original
> > pointer, we should use a VIR_STEAL_PTR just before the cleanup section, so that
> > we don't need an if (ret != 0) or if (ret < 0) check only to conditionally do
> > some cleanup. Feel free to let me know if none of what I just wrote is clear.
>
> I am assuming that you are referring to `group` variable. If so, then I cannot
> apply cleanup attribute to function parameters and `group` is one of them.

I didn't mean using a cleanup attribute. What I meant was making the necessary
adjustments in order to get rid of the 'if(ret != 0)' check, since you're
already touching the code I thought why not going one step further...

Erik

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

On Tue, 24 Jul 2018 at 11:33, Erik Skultety <eskultet@redhat.com> wrote:
>
> On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote:
> > On Mon, 23 Jul 2018 at 16:29, Erik Skultety <eskultet@redhat.com> wrote:
> > >
> > > On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
> > > > Modify virCgroupFree function signature to take a value of type
> > > > virCgroupPtr instead of virCgroupPtr * as the parameter.
> > > >
> > > > Change the argument type in all calls to virCgroupFree function
> > > > from virCgroupPtr * to virCgroupPtr.
> > >
> > > ^This sentence doesn't add any useful information. Instead, the commit message
> > > should add information about why we're performing this change, i.e. in order to
> > > enable usage of VIR_AUTOPTR with cgroup module or something alike.
> > > Also, this patch is oddly placed, IMHO it should come before patch 8, where the
> > > other work on cgroup module is done.
> > >
> > > With that:
> > > Reviewed-by: Erik Skultety <eskultet@redhat.com>
> > >
> > > ...
> > >
> > > > @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path,
> > > >      ret = 0;
> > > >   cleanup:
> > > >      if (ret != 0)
> > > > -        virCgroupFree(group);
> > > > -    virCgroupFree(&parent);
> > > > +        virCgroupFree(*group);
> > > > +    virCgroupFree(parent);
> > >
> > > Since you're already touching the code, I'd appreciate another "adjustment"
> > > patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, IOW
> > > where we're passing a reference to a pointer in order to change the original
> > > pointer, we should use a VIR_STEAL_PTR just before the cleanup section, so that
> > > we don't need an if (ret != 0) or if (ret < 0) check only to conditionally do
> > > some cleanup. Feel free to let me know if none of what I just wrote is clear.
> >
> > I am assuming that you are referring to `group` variable. If so, then I cannot
> > apply cleanup attribute to function parameters and `group` is one of them.
>
> I didn't mean using a cleanup attribute. What I meant was making the necessary
> adjustments in order to get rid of the 'if(ret != 0)' check, since you're
> already touching the code I thought why not going one step further...


You mean something like this?

    VIR_AUTOPTR(virCgroup) ptr = NULL;
    ...
    return 0;
 cleanup:
    VIR_STEAL_PTR(ptr, *group);
    return -1;

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