Re: [libvirt] [PATCH 1/3] util: cgroups do not implicitly add task to new machine cgroup

On 11/13/2015 11:56 AM, Henning Schild wrote: > virCgroupNewMachine used to add the pidleader to the newly created > machine cgroup. Do not do this implicit anymore. > > Signed-off-by: Henning Schild <henning.schild(a)siemens.com&gt; > --- > src/lxc/lxc_cgroup.c | 6 ++++++ > src/qemu/qemu_cgroup.c | 6 ++++++ > src/util/vircgroup.c | 22 ---------------------- > 3 files changed, 12 insertions(+), 22 deletions(-) > > diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c > index ad254e4..e5ac893 100644 > --- a/src/lxc/lxc_cgroup.c > +++ b/src/lxc/lxc_cgroup.c > @@ -504,6 +504,12 @@ virCgroupPtr > virLXCCgroupCreate(virDomainDefPtr def, &cgroup) < 0) > goto cleanup; > > + if (virCgroupAddTask(cgroup, initpid) < 0) { > + virCgroupRemove(cgroup); > + virCgroupFree(&cgroup); > + goto cleanup; > + } > + For both this and qemu, the store/restore last error: virErrorPtr saved = virSaveLastError(); ... if (saved) { virSetError(saved); virFreeError(saved); } Is "lost". I realize no other call to virCgroupRemove saves the error, but as I found in a different review:

http://www.redhat.com/archives/libvir-list/2015-October/msg00823.html the call to virCgroupPathOfController from virCgroupRemove could overwrite the last error. Even though others don't have it, I think perhaps we should ensure it still exists here. Or perhaps a patch prior to this one that would adjust the virCgroupRemove to "save/restore" the last error around the virCgroupPathOfController call... > /* setup control group permissions for user namespace */ > if (def->idmap.uidmap) { > if (virCgroupSetOwner(cgroup, > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c > index a8e0b8c..28d2ca2 100644 > --- a/src/qemu/qemu_cgroup.c > +++ b/src/qemu/qemu_cgroup.c > @@ -769,6 +769,12 @@ qemuInitCgroup(virQEMUDriverPtr driver, > goto cleanup; > } > > + if (virCgroupAddTask(priv->cgroup, vm->pid) < 0) { > + virCgroupRemove(priv->cgroup); > + virCgroupFree(&priv->cgroup); > + goto cleanup; > + } > + > done: > ret = 0; > cleanup: > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > index 0379c2e..a07f3c2 100644 > --- a/src/util/vircgroup.c > +++ b/src/util/vircgroup.c > @@ -1669,16 +1669,6 @@ virCgroupNewMachineSystemd(const char *name, > } > } > > - if (virCgroupAddTask(*group, pidleader) < 0) { > - virErrorPtr saved = virSaveLastError(); > - virCgroupRemove(*group); > - virCgroupFree(group); > - if (saved) { > - virSetError(saved); > - virFreeError(saved); > - } > - } > - > ret = 0; > cleanup: > virCgroupFree(&parent); > @@ -1701,7 +1691,6 @@ int virCgroupTerminateMachine(const char > *name, static int > virCgroupNewMachineManual(const char *name, > const char *drivername, > - pid_t pidleader, > const char *partition, > int controllers, > virCgroupPtr *group) > @@ -1727,16 +1716,6 @@ virCgroupNewMachineManual(const char *name, > group) < 0) > goto cleanup; > > - if (virCgroupAddTask(*group, pidleader) < 0) { > - virErrorPtr saved = virSaveLastError(); > - virCgroupRemove(*group); > - virCgroupFree(group); > - if (saved) { > - virSetError(saved); > - virFreeError(saved); > - } > - } > - > done: > ret = 0; > > @@ -1783,7 +1762,6 @@ virCgroupNewMachine(const char *name, > > return virCgroupNewMachineManual(name, > drivername, > - pidleader, > partition, > controllers, > group); > Beyond that - things seem reasonable. I usually defer to Martin or Peter for cgroup stuff though... Another thought/addition/change would be to have virCgroupNewMachine return 'cgroup' rather than have it as the last parameter and then check vs. NULL for success/failure rather than 0/-1... Weak ACK - hopefully either Peter/Martin can look. I think Peter in particular may be interested due to upcoming vCpu changes. John