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@xxxxxxxxxx>
> > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > ---
> > 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.
Pavel
> John
>
>
> >
> > return 0;
> > +
> > + error:
> > + saved = virSaveLastError();
> > + virCgroupRemove(*group);
> > + virCgroupFree(group);
> > + if (saved) {
> > + virSetError(saved);
> > + virFreeError(saved);
> > + }
> > +
> > + return -1;
> > }
> >
> >
> >
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
