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

[Henning Schild]

On Tue, 8 Dec 2015 12:23:14 -0500
John Ferlan <jferlan at redhat.com> wrote:

> 
> 
> 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 at siemens.com>
> > ---
> >  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:

Yes that was lost and i will get it back in. Further discussions on
where it should be are out of the scope of this series.

> 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