[PATCH] virt-admin: Fix segfault when libvirtd dies
vshAdmCatchDisconnect requires non-NULL structure vshControl for getting connection name (stored at opaque), but virAdmConnectRegisterCloseCallback at vshAdmConnect called it with NULL. Signed-off-by: Adam Julis --- tools/virt-admin.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 37bc6fe4f0..0766032e4a 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -112,7 +112,7 @@ vshAdmConnect(vshControl *ctl, unsigned int flags) return -1; } else { if (virAdmConnectRegisterCloseCallback(priv->conn, vshAdmCatchDisconnect, - NULL, NULL) < 0) + ctl, NULL) < 0) vshError(ctl, "%s", _("Unable to register disconnect callback")); if (priv->wantReconnect) -- 2.43.2 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH] virt-admin: Fix segfault when libvirtd dies
On 3/19/24 12:02, Adam Julis wrote: > vshAdmCatchDisconnect requires non-NULL structure vshControl for > getting connection name (stored at opaque), but > virAdmConnectRegisterCloseCallback at vshAdmConnect called it > with NULL. > > Signed-off-by: Adam Julis > --- > tools/virt-admin.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/virt-admin.c b/tools/virt-admin.c > index 37bc6fe4f0..0766032e4a 100644 > --- a/tools/virt-admin.c > +++ b/tools/virt-admin.c > @@ -112,7 +112,7 @@ vshAdmConnect(vshControl *ctl, unsigned int flags) > return -1; > } else { > if (virAdmConnectRegisterCloseCallback(priv->conn, > vshAdmCatchDisconnect, > - NULL, NULL) < 0) > + ctl, NULL) < 0) > vshError(ctl, "%s", _("Unable to register disconnect callback")); > > if (priv->wantReconnect) Hi, if that is the case I would then expect that virAdmConnectRegisterCloseCallback() needs to also be updated with: +virCheckNonNullArgGoto(opaque, error); or something like that. Is there a reason why it isn't? We better catch the error early if the API is used wrongly. Claudio ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH] virt-admin: Fix segfault when libvirtd dies
On Tue, Mar 19, 2024 at 14:34:08 +0100, Claudio Fontana wrote: > On 3/19/24 12:02, Adam Julis wrote: > > vshAdmCatchDisconnect requires non-NULL structure vshControl for > > getting connection name (stored at opaque), but > > virAdmConnectRegisterCloseCallback at vshAdmConnect called it > > with NULL. > > > > Signed-off-by: Adam Julis > > --- > > tools/virt-admin.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/virt-admin.c b/tools/virt-admin.c > > index 37bc6fe4f0..0766032e4a 100644 > > --- a/tools/virt-admin.c > > +++ b/tools/virt-admin.c > > @@ -112,7 +112,7 @@ vshAdmConnect(vshControl *ctl, unsigned int flags) > > return -1; > > } else { > > if (virAdmConnectRegisterCloseCallback(priv->conn, > > vshAdmCatchDisconnect, > > - NULL, NULL) < 0) > > + ctl, NULL) < 0) > > vshError(ctl, "%s", _("Unable to register disconnect > > callback")); > > > > if (priv->wantReconnect) > > Hi, > > if that is the case I would then expect that > virAdmConnectRegisterCloseCallback() needs to also be updated with: > > +virCheckNonNullArgGoto(opaque, error); > > or something like that. Is there a reason why it isn't? We better catch the > error early if the API is used wrongly. Well, vshAdmCatchDisconnect requires opaque to be non-NULL, but other callbacks registered with virAdmConnectRegisterCloseCallback may not need any opaque data. Jirka ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH] virt-admin: Fix segfault when libvirtd dies
On a Tuesday in 2024, Adam Julis wrote: vshAdmCatchDisconnect requires non-NULL structure vshControl for getting connection name (stored at opaque), but virAdmConnectRegisterCloseCallback at vshAdmConnect called it with NULL. Signed-off-by: Adam Julis --- tools/virt-admin.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH] virt-admin: Fix segfault when libvirtd dies
On 3/19/24 15:58, Jiri Denemark wrote: > On Tue, Mar 19, 2024 at 14:34:08 +0100, Claudio Fontana wrote: >> On 3/19/24 12:02, Adam Julis wrote: >>> vshAdmCatchDisconnect requires non-NULL structure vshControl for >>> getting connection name (stored at opaque), but >>> virAdmConnectRegisterCloseCallback at vshAdmConnect called it >>> with NULL. >>> >>> Signed-off-by: Adam Julis --- >>> tools/virt-admin.c | 2 +- 1 file changed, 1 insertion(+), 1 >>> deletion(-) >>> >>> diff --git a/tools/virt-admin.c b/tools/virt-admin.c index >>> 37bc6fe4f0..0766032e4a 100644 --- a/tools/virt-admin.c +++ >>> b/tools/virt-admin.c @@ -112,7 +112,7 @@ vshAdmConnect(vshControl >>> *ctl, unsigned int flags) return -1; } else { if >>> (virAdmConnectRegisterCloseCallback(priv->conn, >>> vshAdmCatchDisconnect, - >>> NULL, NULL) < 0) + >>> ctl, NULL) < 0) vshError(ctl, "%s", _("Unable to register >>> disconnect callback")); >>> >>> if (priv->wantReconnect) >> >> Hi, >> >> if that is the case I would then expect that >> virAdmConnectRegisterCloseCallback() needs to also be updated >> with: >> >> +virCheckNonNullArgGoto(opaque, error); >> >> or something like that. Is there a reason why it isn't? We better >> catch the error early if the API is used wrongly. > > Well, vshAdmCatchDisconnect requires opaque to be non-NULL, but > other callbacks registered with virAdmConnectRegisterCloseCallback > may not need any opaque data. > Fair enough. Reviewed-by: Claudio Fontana ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org