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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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:

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]