Date: Fri, 15 May 2009 21:49:33 +0100 (BST) From: Iain Hibbert <plunky@rya-online.net> To: Maksim Yevmenkin <maksim.yevmenkin@gmail.com> Cc: freebsd-bluetooth@freebsd.org Subject: Re: libhci update Message-ID: <1242420574.009085.2429.nullmailer@galant.ukfsn.org> In-Reply-To: <bb4a86c70905151021u2a42dd29m2edce0a011aba838@mail.gmail.com> References: <E1Lv5La-00058x-HH@smtpbarns01> <bb4a86c70904211651m6127745ao9d4f26c91e428994@mail.gmail.com> <1240386569.369073.696.nullmailer@galant.ukfsn.org> <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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 15 May 2009, Maksim Yevmenkin wrote:
> On Thu, May 14, 2009 at 2:15 PM, Iain Hibbert <plunky@rya-online.net> wrote:
> > Hi Max,
> > some more queries I'm not sure about..
> >
> > --- hci.c.orig 2009-05-14 21:11:04.000000000 +0100
> > +++ hci.c 2009-05-14 21:26:57.000000000 +0100
> > @@ -118,7 +118,7 @@ bt_devsend(int s, uint16_t opcode, void
> > h.length = 0;
> >
> > while (writev(s, iv, ivn) < 0) {
> > - if (errno == EAGAIN || errno == EINTR)
> > + if (errno == EINTR)
> > continue;
> >
> > return (-1);
> > @@ -163,12 +163,15 @@ bt_devrecv(int s, void *buf, size_t size
> > }
> >
> > while ((n = read(s, buf, size)) < 0) {
> > - if (errno == EAGAIN || errno == EINTR)
> > + if (errno == EINTR)
> > continue;
> >
> > return (-1);
> >
> > 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.
> [....]
>
> > + if (n == 0)
> > + return (0);
> > +
> >
> > the read() in bt_devrecv() can return 0 for EOF at least. ok it shouldn't
> > happen but do you think it would be proper to return it? (or call it an
> > error? I dunno)
>
> can it really return 0? i mean specifically when reading from
> bluetooth socket not in general. in any case the existing code will
> return EIO, i.e. error. granted, it will try to parse data in buf,
> but it will always fail if n == 0.
Well I thought perhaps if a (eg) USB device was removed but it turns out
on NetBSD that doesn't cause the socket to EOF, it will just error out on
sending, perhaps I'm being too careful with the edge cases..
> > @@ -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;
}
> > Also, I'm thinking of the case where r->event=0 but a command_status is
> > returned. If status != 0, an error is produced but if status == 0 then we
> > should set rlen = 0 and return success?
>
> what is wrong with the existing code? it simply returns status code in
> the return parameters and let caller deal with it. why give special
> treatment to status == 0 code? as far as bt_devreq() is concern,
> request has completed, i.e. we send something to the device and we
> got something back. since caller gave us no specifics (i.e. r->event
> == 0), caller should deal with the return parameters.
No I think you right I was misreading that, too many nestings :)
> > @@ -504,10 +510,16 @@ wait_for_more:
> >
> > 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.
static void
bt__devresult(struct bt_devinquiry *ii, int *count, bdaddr_t *ba, uint8_t psrm,
uint8_t pspm, uint8_t *cl, uint16_t co, int8_t rssi, uint8_t *data)
{
int n;
for (n = 0; n < *count; n++)
if (bdaddr_same(&ii[n].bdaddr, ba))
break;
bdaddr_copy(&ii[n].bdaddr, ba);
if (cl != NULL)
memcpy(ii[n].dev_class, cl, HCI_CLASS_SIZE);
ii[n].pscan_rep_mode = psrm;
ii[n].pscan_period_mode = pspm;
ii[n].clock_offset = le16toh(co);
ii[n].rssi = rssi;
if (data != NULL)
memcpy(ii[n].data, data, 240);
if (n == *count)
(*count)++;
}
(I don't really like massive arglists but its better than duplicating the
code three times for inquiry_result, rssi_result and extended_result)
> > @@ -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?
> > 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?
iain
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1242420574.009085.2429.nullmailer>