Date: Mon, 18 May 2009 09:26:34 -0700 From: Maksim Yevmenkin <maksim.yevmenkin@gmail.com> To: Iain Hibbert <plunky@rya-online.net> Cc: freebsd-bluetooth@freebsd.org Subject: Re: libhci update Message-ID: <bb4a86c70905180926x1cb3f79dsad037f06dff4b8c5@mail.gmail.com> In-Reply-To: <1242420574.009085.2429.nullmailer@galant.ukfsn.org> References: <E1Lv5La-00058x-HH@smtpbarns01> <bb4a86c70904220909j5d047ce6x6260bd2e87b5b7bd@mail.gmail.com> <1242294653.314348.1748.nullmailer@galant.ukfsn.org> <bb4a86c70905140926n488cb2b5x5f5530e01d70bd66@mail.gmail.com> <1242322384.832849.4269.nullmailer@galant.ukfsn.org> <bb4a86c70905141140u2b1662fdrf528c157d5fe7c2e@mail.gmail.com> <1242328962.345875.22296.nullmailer@galant.ukfsn.org> <1242335731.252131.19040.nullmailer@galant.ukfsn.org> <bb4a86c70905151021u2a42dd29m2edce0a011aba838@mail.gmail.com> <1242420574.009085.2429.nullmailer@galant.ukfsn.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Iain,
[...]
>> > I think these two should not have EAGAIN because that can be returned if
>> > the socket is set to nonblocking and we should just pass it back to the
>> > caller rather than looping?
>>
>> i somewhat disagree. it is true that _internally_ libbluetooth does
>> not set hci socket to the non-blocking mode. however,
>> bt_devsend/recv() can be used with the hci socket that was created
>> outside of libbluetooth.
>
> Yes, thats the case I meant - actually I see the manpage says it should be
> used with socket created by bt_devopen() but in the case where a socket
> has NBIO set, bt_devrecv() will end up crazyfast looping (==bad) rather
> than do poll/return as read() normally would if there is no data.
oh, i see. you are right. we need to remove EAGAIN completely from
everywhere in libbluetooth and only check for EINTR. i already have
done it in my local tree.
[...]
>> > @@ -302,10 +307,11 @@ bt_devreq(int s, struct bt_devreq *r, ti
>> > if (e->event == r->event) {
>> > if (r->rlen >= n) {
>> > r->rlen = n;
>> > memcpy(r->rparam, e + 1, r->rlen);
>> > }
>> > + /* else what? */
>> >
>> > these three locations in bt_devreq() I'm not sure what should happen if
>> > the provided buffer was not big enough. I suggest the following construct
>> > instead:
>> >
>> > if (r->rlen > n)
>> > r->rlen = n;
>> >
>> > if (r->rlen > 0)
>> > memcpy(r->rparam, ..., r->rlen);
>> >
>> > which gives them as much as they asked for and discards the rest, how
>> > would that look?
>>
>> i did not really know what to do here. practically, this just should
>> not happen. caller is expected to know how big return parameters are
>> and pass appropriately sized buffer. if the caller really does not
>> care about return parameters, then no buffer should be passed.
>> returning something that is incomplete is a bit broken, imo. i
>> actually wrote the code like you suggested, but then decided not to do
>> it this way. cant remember why :) i also considered to return EMSGSIZE
>> in this case, but then deiced not do to it either. again i cant recall
>> the exact reason :)
>>
>> since this is a relatively minor issue, lets agree on something and
>> move on. i could go either way.
>
> Well, there was a case of bad bluetooth device I saw mentioned once that
> gives the inquiry_rssi_result with the wrong packet size but I can't
> remember the details, perhaps you are right - in fact bt_devrecv()
> explicitly makes sure that you have the complete packet so perhaps this
> should just be the same. I think it should probably return an error (EIO)
> though? eg
>
> if (r->rlen >= n) {
> r->rlen = n;
> memcpy(r->rparam, ..., r->rlen);
> } else if (r->rlen > 0) {
> error = EIO;
> goto out;
> }
i would prefer EMSGSIZE, i.e. what i've done here locally is
==
if (r->rlen >= n) {
r->rlen = n;
memcpy(r->rparam, e + 1, r->rlen);
- }
+ } else if (r->rlen > 0)
+ error = EMSGSIZE;
goto out;
==
[...]
>> > case NG_HCI_EVENT_INQUIRY_RESULT:
>> > ir = (ng_hci_inquiry_response *)(ep + 1);
>> >
>> > for (n = 0; n < MIN(ep->num_responses, num_rsp); n ++) {
>> >
>> > I found while writing the inquiry routine in btconfig(8) that some devices
>> > don't filter repeat addresses properly (or maybe its that some devices
>> > continue to respond, I've seen that too). Perhaps its worth handling that
>> > case inside the bt_devinquiry() function by discarding dupes?
>>
>> yes, i've seen that too. broadcom chips (at least in my case) have
>> this behavior. how do you propose to detect the dupes? using bd_addr
>> only? or match all/some parameters as well? i think it would be ok to
>> remove dupes.
>
> No just check bdaddr as it should be the same device. I've come up with
> the following function to add results to the list.. to be called with
> whichever data is present in relevant response and it overwrites the
> earlier entry in case anything changed.
right, lets suppress the dupes in inquiry results. lets only match
bd_addr and do not match any other parameters.
>> > @@ -551,11 +563,11 @@ bt_devinfo(struct bt_devinfo *di)
>> > return (-1);
>> > }
>> >
>> > s = bt_devopen(di->devname);
>> > if (s < 0)
>> > return (-1);
>> >
>> > I'm not sure if in FreeBSD you can connect() to a device that is not
>> > marked up, but in NetBSD you can't. In any case, there is information in
>> > the devinfo structure that is only available from activated devices so it
>> > may fail?
>>
>> in freebsd, you can. bind()/connect() just sets device name in
>> socket's pcb. in fact, in freebsd, you can bind/connect() hci socket
>> to anything :)
>
> yeah I let bind do any address since if it doesn't exist then you won't
> get any messages but connect seemed wrong to connect to nothing. sendto()
> also fails if the destination is unknown..
>
> I presume that for inactive devices things like features, bdaddr and
> buffer_size results will just be nil?
depends what state node is in. there might be some residual data left,
however, this really should not happen unless someone is messing with
the nodes by hand (basically disconnect netgraph hooks) or
intentionally misconfigured the system. in any case, state parameter
should be checked first before accessing anything else. i'm guessing
we need to define bluetooth device states independent of
implementation. in freebsd we have connected, initialized and ready =
connected+ready states. connected means that device hook is connected
(i.e. device is present). initialized means that we run initialization
sequence (get bd_addr, features, etc.).
>> > This really relates to bt_devenum() as below, are we counting 'active'
>> > devices or 'all' devices, as you ignored errors?
>>
>> in freebsd, we return the list of netgraph nodes. that is, device may
>> be present (node exists) but not 'up'. that is what 'state' parameter
>> in devinfo structure is telling you. so, in freebsd, devenum() will
>> return the list of all devices.
>
> Mm, I've fixed bt_devinfo() so it will work for inactive devices, will
> think about the devenum some more. What is the 'state' parameter
> containing is it just 0 = down, 1 = up or is there more?
>
> I think devenum should probably return all devs as caller can check the
> status but I'm not sure if its useful to have a socket connected to an
> empty device and know no details in the down case.. what happens if you
> send hci commands to a down device, nothing?
like i said, we have connected and initialized states. as long as
device is in the 'connected' state we will should a response from it.
from your other email.
> - in bt_devany_cb() you should perhaps make sure that the device is an
> enabled one?
what does 'enabled' means?
> - I'm thinking that bt_devopen() should have an options argument, in order
> to pre-set any options such as TIMESTAMP, DIRECTION etc (even NBIO)
> which will get around the differences in API for that (BlueZ had a weird
> thing with not supporting SOL_SOCKET, SO_TIMESTAMP even though Linux
> does I don't know if they fixed it yet)
maybe bt_dev{get,set}opts() ?
> - in bt_devinquiry() we accept (dev == NULL) to mean any device, should
> that happen in bt_devopen() too?
we could.
> - along this line I wonder if it is possible open a promiscuous socket
> (eg as used by hcidump) instead of any particular device? (could be
> dev==NULL I guess, or a PROMISCUOUS option?) I'm not sure how Linux
> works but on NetBSD you must explicitly bind to 00:00:00:00:00:00 to
> get that behaviour (IIRC FreeBSD gives it to you by default but I was
> paranoid :)
right, in freebsd we just don't bind socket to anything and use filter
to enable all event/packets/etc. this is how we get promiscuous
socket. perhaps bt_devopen(NULL) should mean create non-bound socket?
thanks,
max
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?bb4a86c70905180926x1cb3f79dsad037f06dff4b8c5>