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

[Pavel Hrdina]

On Thu, Sep 27, 2018 at 04:01:08PM +0200, Pavel Hrdina wrote:
> On Thu, Sep 27, 2018 at 08:40:03AM -0400, John Ferlan wrote:
> > 
> > 
> > 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.
> 
> If virCgroupAddProcess fails virCgroupNewMachineManual returns 0 and
> NULL, however, qemuExtTPMSetupCgroup returns -1 and NULL.
> 
> > For qemuExtTPMSetupCgroup one must go back through to
> > qemuSetupCgroupForExtDevices via qemuExtDevicesSetupCgroup to see that
> > the virCgroupRemove is not made on the error path.
> 
> The difference with qemuExtDevicesSetupCgroup is adding another process
> to existing cgroups that were created for the domain itself.  The
> cleanup is done for the whole domain.  If you follow the cleanup path:
> 
> qemuSetupCgroupForExtDevices fails -> goto cleanup, there is no
> qemuRemoveCgroup or virCgroupRemove, this will result that
> qemuProcessLaunch fails -> goto stop, there we will call qemuProcessStop
> where we will call qemuRemoveCgroup which will remove cgroups.
> 
> So that part of code is correct, we don't want to remove cgroups if the
> qemu process is running, we will do it later once we stop the qemu
> process.
> 
> > 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.
> 
> Before this patch:
> 
>     - when virCgroupEnableMissingControllers failed we returned -1 and
>       cgroup ptr or NULL
> 
>     - when virCgroupAddTask failed we returned 0 and NULL
> 
> After this patch:
> 
>     - when virCgroupEnableMissingControllers fails we return -1 and NULL
> 
>     - when virCgroupAddTask fails we return -1 and NULL
> 
> The change for virCgroupEnableMissingControllers is not an issue because
> we always return -1 therefore starting guest always fails and we always 
> call virCgroupRemove.
> 
> The change for virCgroupAddTask changed the behavior, previously we
> continued with starting the guest but no cgroups were used for that
> guest, after this patch starting guest fails.
> 
> So there are two issues after this patch, we need to return 0 if
> virCgroupAddTask fails and we need to modify virCgroupRemove to accept
> NULL as well.
> 
> Marc's patch fixes the second issue, but is incorrect since we tend to
> have check for NULL inside the function.
> 
> I'll post new patches to address both issues.

So we can actually revert this patch because if
virCgroupEnableMissingControllers fails the group is always NULL
therefore there is no need to call virCgroupRemove or virCgroupFree.

I'll post the revert shortly.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180927/43c4a511/attachment-0001.sig>