| Message ID | 20210203165421.1550-2-bouyer@netbsd.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xenstored: close socket connections on error | expand |
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.
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.
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.
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
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 ?
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
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.
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)
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(+)