[libvirt] [PATCH v2 1/9] vircgroup: cleanup controllers not managed by systemd on error

[John Ferlan]

On 9/20/18 4:54 AM, Pavel Hrdina wrote:
> If virCgroupEnableMissingControllers() fails it could already create
> some directories, we should clean it up as well.
> 
> Reviewed-by: Fabiano Fidêncio <fidencio at redhat.com>
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  src/util/vircgroup.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 

Rather than usurp Marc's patch to fix a bug introduced here, I figured
I'd go directly to this patch...

> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index c825f7c77e..f9e387c86d 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1551,6 +1551,7 @@ virCgroupNewMachineSystemd(const char *name,
>      int rv;
>      virCgroupPtr init;
>      VIR_AUTOFREE(char *) path = NULL;
> +    virErrorPtr saved = NULL;
>  
>      VIR_DEBUG("Trying to setup machine '%s' via systemd", name);
>      if ((rv = virSystemdCreateMachine(name,
> @@ -1584,20 +1585,24 @@ virCgroupNewMachineSystemd(const char *name,
>  
>      if (virCgroupEnableMissingControllers(path, pidleader,
>                                            controllers, group) < 0) {
> -        return -1;
> +        goto error;
>      }
>  
> -    if (virCgroupAddTask(*group, pidleader) < 0) {
> -        virErrorPtr saved = virSaveLastError();
> -        virCgroupRemove(*group);
> -        virCgroupFree(group);
> -        if (saved) {
> -            virSetError(saved);
> -            virFreeError(saved);
> -        }
> -    }
> +    if (virCgroupAddTask(*group, pidleader) < 0)
> +        goto error;

This code changes from returning 0 with *group == NULL to returning -1.
Which perhaps isn't a problem per se...

However, I note other return paths from virCgroupNewMachineManual and
qemuExtTPMSetupCgroup when processing a failed virCgroupAddProcess (what
virCgroupAddTask turns into) which still return 0 and a NULL group.

For qemuExtTPMSetupCgroup one must go back through to
qemuSetupCgroupForExtDevices via qemuExtDevicesSetupCgroup to see that
the virCgroupRemove is not made on the error path.

For some additional data, if one goes back through history of this hunk
of code it wasn't added with returning an error (commit 2fe2470181), but
there was a change (commit 71ce47596) that got reverted (commit
d41bd0959) that had changed to returning an error and I have a faint
recollection of that being a problem...

Looking at the archive of the revert patches:

v2:
https://www.redhat.com/archives/libvir-list/2016-January/msg00619.html

v1:
https://www.redhat.com/archives/libvir-list/2016-January/msg00511.html

There's quite a bit of "history" in the v1 patches that may help show
the depth of the issue.

I'm not really sure of the best course of action right now, but I
figured I should at least note it and think about it now while we still
have a chance before the release.

John


>  
>      return 0;
> +
> + error:
> +    saved = virSaveLastError();
> +    virCgroupRemove(*group);
> +    virCgroupFree(group);
> +    if (saved) {
> +        virSetError(saved);
> +        virFreeError(saved);
> +    }
> +
> +    return -1;
>  }
>  
>  
>