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

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(a)redhat.com&gt; > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > 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; > } > > > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list