Re: [Bluez-devel] sdpd in polling loop on disconnect

[Frank de Lange <frank@unternet.org>]

[-- Attachment #1: Type: text/plain, Size: 3310 bytes --]

Marcel Holtmann wrote:
> if this is bluez-utils-3.9 you might wanna try to compile it with
> --enable-glib to use the system Glib. It might be possible that our
> eglib has some kind of bad. In case of using the Glib system library, I
> think you discovered a real bug in sdpd. Do you see the same issue if
> you don't start sdpd and use hcid -s instead.

Fedora's bluez-utils is already compiled with --enable-glib:

%configure --with-bluez-libs=%{_libdir} --enable-pie --enable-debug \
        --enable-all --disable-bcm203x --enable-alsa --enable-bccmd \
        --enable-bccmd --enable-avctrl --enable-glib

Trying with hcid -s instead of a separate sdpd leads to the same
behaviour: hcid loops on poll.

Looking at the code it strikes me that there are many calls to
g_io_add_watch which only trigger on G_IO_IN or G_IO_OUT. These channels
are configured with g_io_channel_set_close_on_unref(<channel>, TRUE) but
that does not seem to be enough to set POLLHUP (or POLLRDHUP) on the
listening socket. What seems to happen is that the socket is kept open 
after the remote connection dropped and polled continuously, most likely 
returning POLLRDHUP. The semantics of glib are a bit unclear in this 
respect but I have found the following quote from Owen Taylor:

(http://www.mail-archive.com/gtk-devel-list@gnome.org/msg01577.html)

"In general, on systems new enough to have poll(), a socket will
indicate HUP rather than IN when it has been closed on the other
end. poll() and select() are about *state* rather than *events*
so once a socket returns HUP, it will continue to return HUP.

On the other hand, if a system is emulating poll() with select() -
which was the standard state 5 years ago and I think is still
the case on OS X, then you'll just get IN, and have to notice the
zero length read.

In general, if you are writing code that you want to be portable,
what you should do is add a watch on G_IO_IN | G_IO_HUP, and
in that callback just always do a read. If the read is zero length,
then the socket has been closed, so cleanup, close the socket,
and return FALSE to remove the watch."

I have patched sdpd to add G_IO_HUP|G_IO_ERR to the watched conditions 
on the g_io_channel. This cures the polling loop behaviour. There are 
several other places in the code where channels are being watched for 
G_IO_IN only. Someone with good knowledge of the context in which these 
g_io_channels are being used should check whether there is a need for 
adding G_IO_HUP to the watched conditions. The callbacks should also be 
changed to watch for this condition as failing to do the latter seems to 
lead to kernel panics. This is a bug in its own right - a userspace 
program should not be able to bring down the kernel... I have not found 
the culprit for those panics yet.

Attached: patch for sdpd (adds G_IO_HUP|G_IO_ERR to watched conditions 
on g_io_channels)

Cheers//Frank
-- 
    WWWWW      ________________________
   ## o o\    /     Frank de Lange     \
   }#   \|   /                          \
    \ `--| _/     <Hacker for Hire>      \
     `---'  \       +46-734352015        /
             \    frank@unternet.org    /
              `------------------------'
   [ "Omnis enim res, quae dando non deficit, dum habetur
      et non datur, nondum habetur, quomodo habenda est."  ]



[-- Attachment #2: bluez-utils.g_io_add_watch-g_io_hup.patch --]
[-- Type: text/x-patch, Size: 1160 bytes --]

diff -pruN bluez-utils-3.9/sdpd/server.c bluez-utils-3.9.patched/sdpd/server.c
--- bluez-utils-3.9/sdpd/server.c	2007-01-21 19:57:15.000000000 +0100
+++ bluez-utils-3.9.patched/sdpd/server.c	2007-02-19 20:32:35.000000000 +0100
@@ -207,7 +207,7 @@ static gboolean io_accept_event(GIOChann
 	io = g_io_channel_unix_new(nsk);
 	g_io_channel_set_close_on_unref(io, TRUE);
 
-	g_io_add_watch(io, G_IO_IN, io_session_event, data);
+	g_io_add_watch(io, G_IO_IN|G_IO_HUP|G_IO_ERR, io_session_event, data);
 
 	g_io_channel_unref(io);
 
@@ -230,13 +230,13 @@ int start_sdp_server(uint16_t mtu, uint3
 	l2cap_io = g_io_channel_unix_new(l2cap_sock);
 	g_io_channel_set_close_on_unref(l2cap_io, TRUE);
 
-	g_io_add_watch(l2cap_io, G_IO_IN, io_accept_event, &l2cap_sock);
+	g_io_add_watch(l2cap_io, G_IO_IN|G_IO_HUP|G_IO_ERR, io_accept_event, &l2cap_sock);
 
 	if (compat && unix_sock > fileno(stderr)) {
 		unix_io = g_io_channel_unix_new(unix_sock);
 		g_io_channel_set_close_on_unref(unix_io, TRUE);
 
-		g_io_add_watch(unix_io, G_IO_IN, io_accept_event, &unix_sock);
+		g_io_add_watch(unix_io, G_IO_IN|G_IO_HUP|G_IO_ERR, io_accept_event, &unix_sock);
 	}
 
 	return 0;

[-- Attachment #3: Type: text/plain, Size: 345 bytes --]

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- Attachment #4: Type: text/plain, Size: 164 bytes --]

_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel