On Tue, 8 Dec 2015 12:23:14 -0500
John Ferlan <jferlan@xxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list