[PATCH] xenstored: close socket connections on error

Manuel Bouyer posted 1 patch 3 years, 1 month ago
Test gitlab-ci passed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20210203165421.1550-2-bouyer@netbsd.org
tools/xenstore/xenstored_core.c | 3 +++
1 file changed, 3 insertions(+)
[PATCH] xenstored: close socket connections on error
Posted by Manuel Bouyer 3 years, 1 month ago
On error, don't keep socket connection in ignored state but close them.
When the remote end of a socket is closed, xenstored will flag it as an
error and switch the connection to ignored. But on some OSes (e.g.
NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored
state will stay open forever in xenstored (and it will loop with CPU 100%
busy).

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
Fixes: d2fa370d3ef9cbe22d7256c608671cdcdf6e0083
---
 tools/xenstore/xenstored_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 1ab6f162cb..0fea598352 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1440,6 +1440,9 @@ static void ignore_connection(struct connection *conn)
 
 	talloc_free(conn->in);
 	conn->in = NULL;
+	/* if this is a socket connection, drop it now */
+	if (conn->fd >= 0)
+		talloc_free(conn);
 }
 
 static const char *sockmsg_string(enum xsd_sockmsg_type type)
-- 
2.29.2


Re: [PATCH] xenstored: close socket connections on error
Posted by Jürgen Groß 3 years, 1 month ago
On 03.02.21 17:54, Manuel Bouyer wrote:
> On error, don't keep socket connection in ignored state but close them.
> When the remote end of a socket is closed, xenstored will flag it as an
> error and switch the connection to ignored. But on some OSes (e.g.
> NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored
> state will stay open forever in xenstored (and it will loop with CPU 100%
> busy).
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> Fixes: d2fa370d3ef9cbe22d7256c608671cdcdf6e0083

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen
Re: [PATCH] xenstored: close socket connections on error
Posted by Manuel Bouyer 3 years, 1 month ago
On Thu, Feb 04, 2021 at 12:11:02PM +0100, Jürgen Groß wrote:
> On 03.02.21 17:54, Manuel Bouyer wrote:
> > On error, don't keep socket connection in ignored state but close them.
> > When the remote end of a socket is closed, xenstored will flag it as an
> > error and switch the connection to ignored. But on some OSes (e.g.
> > NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored
> > state will stay open forever in xenstored (and it will loop with CPU 100%
> > busy).
> > 
> > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> > Fixes: d2fa370d3ef9cbe22d7256c608671cdcdf6e0083
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

thanks.
I still don't know if I'm supposed to send a new version of the patch with
these tags, even if the patch itself doesn't change, or if the commiter
will handle them ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] xenstored: close socket connections on error
Posted by Jürgen Groß 3 years, 1 month ago
On 04.02.21 12:16, Manuel Bouyer wrote:
> On Thu, Feb 04, 2021 at 12:11:02PM +0100, Jürgen Groß wrote:
>> On 03.02.21 17:54, Manuel Bouyer wrote:
>>> On error, don't keep socket connection in ignored state but close them.
>>> When the remote end of a socket is closed, xenstored will flag it as an
>>> error and switch the connection to ignored. But on some OSes (e.g.
>>> NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored
>>> state will stay open forever in xenstored (and it will loop with CPU 100%
>>> busy).
>>>
>>> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
>>> Fixes: d2fa370d3ef9cbe22d7256c608671cdcdf6e0083
>>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> thanks.
> I still don't know if I'm supposed to send a new version of the patch with
> these tags, even if the patch itself doesn't change, or if the commiter
> will handle them ?
> 

Will be done by the committer.


Juergen
Re: [PATCH] xenstored: close socket connections on error
Posted by Ian Jackson 3 years, 1 month ago
Manuel Bouyer writes ("Re: [PATCH] xenstored: close socket connections on error"):
> On Thu, Feb 04, 2021 at 12:11:02PM +0100, Jürgen Groß wrote:
> > On 03.02.21 17:54, Manuel Bouyer wrote:
> > > On error, don't keep socket connection in ignored state but close them.
> > > When the remote end of a socket is closed, xenstored will flag it as an
> > > error and switch the connection to ignored. But on some OSes (e.g.
> > > NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored
> > > state will stay open forever in xenstored (and it will loop with CPU 100%
> > > busy).
> > > 
> > > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> > > Fixes: d2fa370d3ef9cbe22d7256c608671cdcdf6e0083
> > 
> > Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> thanks.
> I still don't know if I'm supposed to send a new version of the patch with
> these tags, even if the patch itself doesn't change, or if the commiter
> will handle them ?

The committer will handle them.

Thanks,
Ian.

Re: [PATCH] xenstored: close socket connections on error
Posted by Ian Jackson 3 years, 1 month ago
Manuel Bouyer writes ("[PATCH] xenstored: close socket connections on error"):
> On error, don't keep socket connection in ignored state but close them.
> When the remote end of a socket is closed, xenstored will flag it as an
> error and switch the connection to ignored. But on some OSes (e.g.
> NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored
> state will stay open forever in xenstored (and it will loop with CPU 100%
> busy).

Juergen, I think you probably know this code the best.  Would you be
able to review this ?

I'm not sure I understand what the specific behaviour on NetBSD is
that is upsetting xenstored.  Or rather, what it is that xenstored is
using to tell when the socket should be closed.

I grepped for POLLERR and nothing came up.

Or to put it another way, is this commit

> Fixes: d2fa370d3ef9cbe22d7256c608671cdcdf6e0083

broken on Linux too ?

Ian.

Re: [PATCH] xenstored: close socket connections on error
Posted by Ian Jackson 3 years, 1 month ago
Ian Jackson writes ("Re: [PATCH] xenstored: close socket connections on error"):
> Manuel Bouyer writes ("[PATCH] xenstored: close socket connections on error"):
> > On error, don't keep socket connection in ignored state but close them.
> > When the remote end of a socket is closed, xenstored will flag it as an
> > error and switch the connection to ignored. But on some OSes (e.g.
> > NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored
> > state will stay open forever in xenstored (and it will loop with CPU 100%
> > busy).
> 
> Juergen, I think you probably know this code the best.  Would you be
> able to review this ?
> 
> I'm not sure I understand what the specific behaviour on NetBSD is
> that is upsetting xenstored.  Or rather, what it is that xenstored is
> using to tell when the socket should be closed.
> 
> I grepped for POLLERR and nothing came up.
> 
> Or to put it another way, is this commit
> 
> > Fixes: d2fa370d3ef9cbe22d7256c608671cdcdf6e0083
> 
> broken on Linux too ?

Andy pointed me to the recent thread "xenstored file descriptor leak"
which answers all these questions.  I think it would have been nice if
some tools maintainer(s) had been CC'd on that :-).

Juergen, I guess I will get a formal R-b from you ?

Release-Acked-by: Ian Jackson <iwj@xenproject.org>


Manuel, in response to this:

> When I started, I looked at the wiki for instructions about
> patches, but didn't find any ...

Earlier I offered you help with git, in private email.  I agree that
git is confusing and sometimes impenetrable.  But it seems that what
you are doing now is worse!  Please take me up on my offer of help.

Our wiki doesn't give instructions on how to use git to maintain a
patch series.  Those instructions would not be Xen-specific.  Perhaps
we could have a pointer or two, but everyone has their own pet methods
and tooling so the result would perhaps be more confusing than
helpful.

Regards,
Ian.

Re: [PATCH] xenstored: close socket connections on error
Posted by Manuel Bouyer 3 years, 1 month ago
On Wed, Feb 03, 2021 at 05:38:59PM +0000, Ian Jackson wrote:
> > [...]
> > broken on Linux too ?
> 
> Andy pointed me to the recent thread "xenstored file descriptor leak"
> which answers all these questions.  I think it would have been nice if
> some tools maintainer(s) had been CC'd on that :-).

I did use add_maintainers.pl against it (or at last it was my intent)

> 
> Juergen, I guess I will get a formal R-b from you ?
> 
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> 
> 
> Manuel, in response to this:
> 
> > When I started, I looked at the wiki for instructions about
> > patches, but didn't find any ...
> 
> Earlier I offered you help with git, in private email.  I agree that
> git is confusing and sometimes impenetrable.  But it seems that what
> you are doing now is worse!  Please take me up on my offer of help.

I didn't forget. It's just that I don't even know what to ask to start
with.  It seems that StGit will help a lot though.

> 
> Our wiki doesn't give instructions on how to use git to maintain a
> patch series.  Those instructions would not be Xen-specific.  Perhaps
> we could have a pointer or two, but everyone has their own pet methods
> and tooling so the result would perhaps be more confusing than
> helpful.

a howto is alwaus helpfull. Even if it's not the one and only way
to do, at last it gives a start point.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--