[PATCH] Fix bogus avdtp error codes management

[PATCH] Fix bogus avdtp error codes management

[Fabien Chevalier]

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

Hi Johan & Marcel,

While trying to fix this cross case issue, i came with an issue with the 
way error codes are handled
in avdtp.c.
The issue is basically that the part of the code that retrieves the 
exact error value is never executed
due to premature return.

Please have a look at the current patch.

Fabien

[-- Attachment #2: avdtp-bogus-error-checking.patch --]
[-- Type: text/x-patch, Size: 1422 bytes --]

diff -u -1 -0 -r1.49 avdtp.c
--- avdtp.c     2 Sep 2007 16:52:50 -0000       1.49
+++ avdtp.c     17 Sep 2007 17:33:24 -0000
@@ -1501,38 +1501,38 @@
        if (cond & G_IO_NVAL)
                return FALSE;
 
        if (!g_slist_find(sessions, session)) {
                debug("l2cap_connect_cb: session got removed");
                return FALSE;
        }
 
        sk = g_io_channel_unix_get_fd(chan);
 
-       if (cond & (G_IO_ERR | G_IO_HUP)) {
-               err = EIO;
-               goto failed;
-       }
-
        len = sizeof(ret);
        if (getsockopt(sk, SOL_SOCKET, SO_ERROR, &ret, &len) < 0) {
                err = errno;
                error("getsockopt(SO_ERROR): %s (%d)", strerror(err), err);
                goto failed;
        }
 
        if (ret != 0) {
                err = ret;
                error("connect(): %s (%d)", strerror(err), err);
                goto failed;
        }
 
+       if (cond & G_IO_HUP) {
+               err = EIO;
+               goto failed;
+       }
+
        ba2str(&session->dst, address);
        debug("AVDTP: connected %s channel to %s",
                        session->pending_open ? "transport" : "signaling",
                        address);
 
        memset(&l2o, 0, sizeof(l2o));
        len = sizeof(l2o);
        if (getsockopt(sk, SOL_L2CAP, L2CAP_OPTIONS, &l2o,
                                &len) < 0) {
                err = errno;
f

[-- Attachment #3: fchevalier.vcf --]
[-- Type: text/x-vcard, Size: 253 bytes --]

begin:vcard
fn:Fabien CHEVALIER
n:CHEVALIER;Fabien
org:SILICOM
adr:;;4 rue de Jouanet; RENNES ATALANTE;;35700;FRANCE
email;internet:fchevalier@silicom.fr
title:Software & Studies Engineer
tel;work:+33 (0) 2 99 84 17 17
version:2.1
end:vcard

Re: [PATCH] Fix bogus avdtp error codes management

[Johan Hedberg]

Hi Fabien,

On Mon, Sep 17, 2007, Fabien Chevalier wrote:
> While trying to fix this cross case issue, i came with an issue with the 
> way error codes are handled
> in avdtp.c.
> The issue is basically that the part of the code that retrieves the 
> exact error value is never executed
> due to premature return.

Looks fine to me if it works. However, I'd like to hear some comment
from Marcel as well. At least my connect(2) manpage implies that a
non-blocking connect should always produce a POLLOUT on connection
completion regardless if the result was failure or success. So it seems
bluez may be behaving differently than other socket types in this
respect.

We had a similar issue with the EAGAIN/EINPROGRESS mixup with
non-blocking connect's so maybe this issue should also be fixed on the
kernel side (however a userspace work-around such as your patch is
probably also desirable).

Johan

Re: [Bluez-devel] [PATCH] Fix bogus avdtp error codes management

[Marcel Holtmann]

Hi Johan,

> > While trying to fix this cross case issue, i came with an issue with the 
> > way error codes are handled
> > in avdtp.c.
> > The issue is basically that the part of the code that retrieves the 
> > exact error value is never executed
> > due to premature return.
> 
> Looks fine to me if it works. However, I'd like to hear some comment
> from Marcel as well. At least my connect(2) manpage implies that a
> non-blocking connect should always produce a POLLOUT on connection
> completion regardless if the result was failure or success. So it seems
> bluez may be behaving differently than other socket types in this
> respect.

if that is so, then we have to fix the kernel. Can someone write me a
simple reproducer or better send me a patch for the kernel.

Regards

Marcel




-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [PATCH] Fix bogus avdtp error codes management

[Fabien Chevalier]

[-- Attachment #1.1: Type: text/plain, Size: 1838 bytes --]

Hi All,

The attached patch makes sure that the correct error code is returned to 
the upper layers.
The issue with the current cvs's code is that EIO would be returned 
whatever the error code returned by the L2CAP layer is, as 
getsockopt(sk, SOL_SOCKET, SO_ERROR, ....) would never be called.
The result of this patch is that you now see the reason of the failure 
in daemon log in case of L2CAP issue, such as:

Sep 18 10:32:53 tannat audio[16934]: avdtp_ref(0x806d230): ref=2
Sep 18 10:32:53 tannat audio[16934]: a2dp_source_request_stream: 
selected SEP 0x806a400
Sep 18 10:32:53 tannat audio[16934]: avdtp_ref(0x806d230): ref=3
Sep 18 10:32:53 tannat audio[16934]: stream creation in progress
Sep 18 10:33:13 tannat audio[16934]: connect(): Host is down (112)


As for the POLLOUT thing, i must say i'm confused..., and don't really 
see the link with the patch :-(
I guess it works already as the GIOChannels heavily rely on it.

Can any of you apply this patch ?

Cheers,

Fabien


> Hi Johan,
>
>   
>>> While trying to fix this cross case issue, i came with an issue with the 
>>> way error codes are handled
>>> in avdtp.c.
>>> The issue is basically that the part of the code that retrieves the 
>>> exact error value is never executed
>>> due to premature return.
>>>       
>> Looks fine to me if it works. However, I'd like to hear some comment
>> from Marcel as well. At least my connect(2) manpage implies that a
>> non-blocking connect should always produce a POLLOUT on connection
>> completion regardless if the result was failure or success. So it seems
>> bluez may be behaving differently than other socket types in this
>> respect.
>>     
>
> if that is so, then we have to fix the kernel. Can someone write me a
> simple reproducer or better send me a patch for the kernel.
>
> Regards
>
> Marcel
>
>
>
>
>   


[-- Attachment #1.2: Type: text/html, Size: 2372 bytes --]

[-- Attachment #2: fchevalier.vcf --]
[-- Type: text/x-vcard, Size: 253 bytes --]

begin:vcard
fn:Fabien CHEVALIER
n:CHEVALIER;Fabien
org:SILICOM
adr:;;4 rue de Jouanet; RENNES ATALANTE;;35700;FRANCE
email;internet:fchevalier@silicom.fr
title:Software & Studies Engineer
tel;work:+33 (0) 2 99 84 17 17
version:2.1
end:vcard

Re: [PATCH] Fix bogus avdtp error codes management

[Johan Hedberg]

On Tue, Sep 18, 2007, Fabien Chevalier wrote:
> As for the POLLOUT thing, i must say i'm confused..., and don't really 
> see the link with the patch :-(
> I guess it works already as the GIOChannels heavily rely on it.

GLib maps POLLOUT to G_IO_OUT, POLLHUP to G_IO_HUP, etc. I was saying
that to conform to how other socket types behave[1] the callback should
*not* be getting a HUP or ERR in this case but a OUT instead. As for
applying the patch, I'm fine with it even though it in essence is a
work-around for a kernel side problem. However, it's Marcel's call in
the end.

[1] I have yet to verify this but that's what the connect(2) manpage
implies (in the section about EINPROGRESS)

Johan

Re: [Bluez-devel] [PATCH] Fix bogus avdtp error codes management

[Fabien Chevalier]

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

Johan Hedberg wrote:
> On Tue, Sep 18, 2007, Fabien Chevalier wrote:
>> As for the POLLOUT thing, i must say i'm confused..., and don't really 
>> see the link with the patch :-(
>> I guess it works already as the GIOChannels heavily rely on it.
> 
> GLib maps POLLOUT to G_IO_OUT, POLLHUP to G_IO_HUP, etc. I was saying
> that to conform to how other socket types behave[1] the callback should
> *not* be getting a HUP or ERR in this case but a OUT instead. As for
> applying the patch, I'm fine with it even though it in essence is a
> work-around for a kernel side problem. However, it's Marcel's call in
> the end.
> 
> [1] I have yet to verify this but that's what the connect(2) manpage
> implies (in the section about EINPROGRESS)

Ok, thanks, this time i understood. :-)
I wrote a test program that does an asynchronous connect using the IP 
stack. (I attached the file, you can dry run yourself if you're curious 
;-) ). On error we receive a revent with POLLERR value, and no POLLOUT.
Conclusion is:
   * POLLOUT is to be sent only when the channel is ready to send more data.
   * POLLERR is the right thing to send when we cannot establish the 
connection due to whatever error ==> The man page is a bit ambiguous 
about it.
   * which means the bt kernel part is fine and does not need fixing
   * which means my patch is not a workaround but a clean fix to this 
bogus error returning code :-)

Marcel, Johan, if you don't have any other remarks, you would mind 
applying the patch ?

Cheers,

Fabien

[-- Attachment #2: main.c --]
[-- Type: text/x-csrc, Size: 882 bytes --]

#include <stdio.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <poll.h>
#include <errno.h>
#include <assert.h>
#include <fcntl.h>

int main()
{
    struct sockaddr_in addr;
    int err;
    struct pollfd pfds;

    int sock = socket(AF_INET, SOCK_STREAM, 0); 

    addr.sin_family = AF_INET;
    addr.sin_port = htons(81);
    addr.sin_addr.s_addr = inet_addr("209.85.135.104");

    fcntl(sock, F_SETFL, fcntl(sock, F_GETFL, 0) | O_NONBLOCK);

    err = connect(sock, (struct sockaddr *)&addr, sizeof(addr));
    
    assert(err == -1 && errno == EINPROGRESS);
    
    pfds.fd = sock;
    pfds.events = POLLOUT;
    
    err = poll(&pfds, 1, -1);
    assert(err == 1);
    
    printf("returned events from poll() routine:");
    if(pfds.revents & POLLOUT)
        printf("POLLOUT");
    if(pfds.revents & POLLERR)
        printf("POLLERR");

    printf("\n");    
}

[-- Attachment #3: fchevalier.vcf --]
[-- Type: text/x-vcard, Size: 242 bytes --]

begin:vcard
fn:Fabien CHEVALIER
n:CHEVALIER;Fabien
org:SILICOM
adr:;;4 rue de Jouanet; RENNES ATALANTE;;35700;FRANCE
email;internet:fchevalier@silicom.fr
title:Software & Studies Engineer
tel;work:+33 (0) 2 99 84 17 17
version:2.1
end:vcard