Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr
On Tue, 24 Jul 2018 at 11:33, Erik Skultety wrote: > > On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote: > > On Mon, 23 Jul 2018 at 16:29, Erik Skultety 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 > > > > > > ... > > > > > > > @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path, > > > > ret = 0; > > > > cleanup: > > > > if (ret != 0) > > > > -virCgroupFree(group); > > > > -virCgroupFree(); > > > > +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
Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr
On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote: > On Mon, 23 Jul 2018 at 16:29, Erik Skultety 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 > > > > ... > > > > > @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path, > > > ret = 0; > > > cleanup: > > > if (ret != 0) > > > -virCgroupFree(group); > > > -virCgroupFree(); > > > +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
Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr
On Mon, 23 Jul 2018 at 16:29, Erik Skultety 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 > > ... > > > @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path, > > ret = 0; > > cleanup: > > if (ret != 0) > > -virCgroupFree(group); > > -virCgroupFree(); > > +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
Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr
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 ... > @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path, > ret = 0; > cleanup: > if (ret != 0) > -virCgroupFree(group); > -virCgroupFree(); > +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
[libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr
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
---
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();
+virCgroupFree(cgroup);
return 0;
error:
virDispatchError(NULL);
-virCgroupFree();
+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();
+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();
+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();
+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(>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(>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(>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();
+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();
+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();
+virCgroupFree(selfcgroup);
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Unable to find 'memory' cgroups controller mount"));
return -1;
}
-virCgroupFree();
+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
