Re: [libvirt] [PATCH] Revert "vircgroup: cleanup controllers not managed by systemd on error"
On Thu, Sep 27, 2018 at 04:13 PM +0200, Pavel Hrdina wrote:
> This reverts commit 1602aa28f820ada66f707cef3e536e8572fbda1e.
>
> There is no need to call virCgroupRemove() nor virCgroupFree() if
> virCgroupEnableMissingControllers() fails because it will not modify
> 'group' at all. The cleanup is done in virCgroupMakeGroup().
>
> Signed-off-by: Pavel Hrdina
> ---
> src/util/vircgroup.c | 25 ++---
> 1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index f90906e4ad..548c873da8 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1055,7 +1055,6 @@ 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,
> @@ -1088,24 +1087,20 @@ virCgroupNewMachineSystemd(const char *name,
>
> if (virCgroupEnableMissingControllers(path, pidleader,
>controllers, group) < 0) {
> -goto error;
> +return -1;
> }
>
> -if (virCgroupAddProcess(*group, pidleader) < 0)
> -goto error;
> +if (virCgroupAddProcess(*group, pidleader) < 0) {
> +virErrorPtr saved = virSaveLastError();
> +virCgroupRemove(*group);
> +virCgroupFree(group);
> +if (saved) {
> +virSetError(saved);
> +virFreeError(saved);
> +}
> +}
>
> return 0;
> -
> - error:
> -saved = virSaveLastError();
> -virCgroupRemove(*group);
> -virCgroupFree(group);
> -if (saved) {
> -virSetError(saved);
> -virFreeError(saved);
> -}
> -
> -return -1;
> }
>
>
> --
> 2.17.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
Reviewed-by: Marc Hartmayer
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Revert "vircgroup: cleanup controllers not managed by systemd on error"
On Thu, Sep 27, 2018 at 04:32:37PM +0200, Fabiano Fidêncio wrote: > On Thu, 2018-09-27 at 16:13 +0200, Pavel Hrdina wrote: > > This reverts commit 1602aa28f820ada66f707cef3e536e8572fbda1e. > > > > There is no need to call virCgroupRemove() nor virCgroupFree() if > > virCgroupEnableMissingControllers() fails because it will not modify > > 'group' at all. The cleanup is done in virCgroupMakeGroup(). > > I wouldn't mention "The cleanup is done in virCGroupMakeGroup() because > there's no cleanup needed from virCgroupEnableMissingControllers() as > groups is only modified in case of success. It's not a cleanup of 'group' variable but cleanup of the directories created on host. > I'd wait for John's ACK as well, but: > Reviewed-by: Fabiano Fidêncio I can modify the commit message to make it more obvious. Pavel signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Revert "vircgroup: cleanup controllers not managed by systemd on error"
On Thu, 2018-09-27 at 16:13 +0200, Pavel Hrdina wrote: > This reverts commit 1602aa28f820ada66f707cef3e536e8572fbda1e. > > There is no need to call virCgroupRemove() nor virCgroupFree() if > virCgroupEnableMissingControllers() fails because it will not modify > 'group' at all. The cleanup is done in virCgroupMakeGroup(). I wouldn't mention "The cleanup is done in virCGroupMakeGroup() because there's no cleanup needed from virCgroupEnableMissingControllers() as groups is only modified in case of success. I'd wait for John's ACK as well, but: Reviewed-by: Fabiano Fidêncio -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Revert "vircgroup: cleanup controllers not managed by systemd on error"
On 9/27/18 10:13 AM, Pavel Hrdina wrote:
> This reverts commit 1602aa28f820ada66f707cef3e536e8572fbda1e.
>
> There is no need to call virCgroupRemove() nor virCgroupFree() if
> virCgroupEnableMissingControllers() fails because it will not modify
> 'group' at all. The cleanup is done in virCgroupMakeGroup().
>
> Signed-off-by: Pavel Hrdina
> ---
> src/util/vircgroup.c | 25 ++---
> 1 file changed, 10 insertions(+), 15 deletions(-)
>
For this patch,
Reviewed-by: John Ferlan
However, of course this tickle's Coverity (that's what happens when
someone makes a change in code).
Anyway, it seems calling virCgroupSetOwner with a NULL cgroup as could
happen in virLXCCgroupCreate if virCgroupNewMachine returns 0 and cgroup
== NULL, will cause a problem.
Yes, this existing and no I'm not knowledgeable (nor is Coverity for
that matter) to know if having def->idmap.uidmap is "related".
John
It's like an onion, peel one layer and we find something else.
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index f90906e4ad..548c873da8 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1055,7 +1055,6 @@ 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,
> @@ -1088,24 +1087,20 @@ virCgroupNewMachineSystemd(const char *name,
>
> if (virCgroupEnableMissingControllers(path, pidleader,
>controllers, group) < 0) {
> -goto error;
> +return -1;
> }
>
> -if (virCgroupAddProcess(*group, pidleader) < 0)
> -goto error;
> +if (virCgroupAddProcess(*group, pidleader) < 0) {
> +virErrorPtr saved = virSaveLastError();
> +virCgroupRemove(*group);
> +virCgroupFree(group);
> +if (saved) {
> +virSetError(saved);
> +virFreeError(saved);
> +}
> +}
>
> return 0;
> -
> - error:
> -saved = virSaveLastError();
> -virCgroupRemove(*group);
> -virCgroupFree(group);
> -if (saved) {
> -virSetError(saved);
> -virFreeError(saved);
> -}
> -
> -return -1;
> }
>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Revert "vircgroup: cleanup controllers not managed by systemd on error"
This reverts commit 1602aa28f820ada66f707cef3e536e8572fbda1e.
There is no need to call virCgroupRemove() nor virCgroupFree() if
virCgroupEnableMissingControllers() fails because it will not modify
'group' at all. The cleanup is done in virCgroupMakeGroup().
Signed-off-by: Pavel Hrdina
---
src/util/vircgroup.c | 25 ++---
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index f90906e4ad..548c873da8 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1055,7 +1055,6 @@ 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,
@@ -1088,24 +1087,20 @@ virCgroupNewMachineSystemd(const char *name,
if (virCgroupEnableMissingControllers(path, pidleader,
controllers, group) < 0) {
-goto error;
+return -1;
}
-if (virCgroupAddProcess(*group, pidleader) < 0)
-goto error;
+if (virCgroupAddProcess(*group, pidleader) < 0) {
+virErrorPtr saved = virSaveLastError();
+virCgroupRemove(*group);
+virCgroupFree(group);
+if (saved) {
+virSetError(saved);
+virFreeError(saved);
+}
+}
return 0;
-
- error:
-saved = virSaveLastError();
-virCgroupRemove(*group);
-virCgroupFree(group);
-if (saved) {
-virSetError(saved);
-virFreeError(saved);
-}
-
-return -1;
}
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
