libhci update

[Maksim Yevmenkin]

On Thu, May 14, 2009 at 2:15 PM, Iain Hibbert <plunky at 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.

[....]

> +       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.

> @@ -273,10 +276,11 @@ bt_devreq(int s, struct bt_devreq *r, ti
>
>                                if (r->rlen >= n) {
>                                        r->rlen = n;
>                                        memcpy(r->rparam, cc + 1, r->rlen);
>                                }
> +                               /* else what? */
>
>                                goto out;
>                        }
>                        break;
>
> @@ -290,10 +294,11 @@ bt_devreq(int s, struct bt_devreq *r, ti
>                                } else {
>                                        if (r->rlen >= n) {
>                                                r->rlen = n;
>                                                memcpy(r->rparam, cs, r->rlen);
>                                        }
> +                                       /* else what? */
>
>                                        goto out;
>                                }
>                        }
>                        break;
> @@ -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.

> 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.

> @@ -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.

> @@ -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 :)

>  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.

> @@ -659,21 +671,24 @@ bt_devenum(bt_devenum_cb_t cb, void *arg
>        }
>
>        for (count = 0, i = 0; i < rp.num_names; i ++) {
>                strlcpy(di.devname, rp.names[i].name, sizeof(di.devname));
>                if (bt_devinfo(&di) < 0)
> -                       continue;
> +                       continue;       /* or maybe error? */
>
>                count ++;
>
>                if (cb == NULL)
>                        continue;
>
>                strlcpy(ha.hci_node, rp.names[i].name, sizeof(ha.hci_node));
>                if (bind(s, (struct sockaddr *) &ha, sizeof(ha)) < 0 ||
>                    connect(s, (struct sockaddr *) &ha, sizeof(ha)) < 0)
> -                       continue;
> +                       continue;       /* or maybe error? */
> +
>
> I think the second one should cause an error return at least? Not sure
> about the first - in NetBSD I have the flags already so I've skipped
> inactive interfaces as below

this is probably something that should be os/stack implementation
specific. however, i'll try very hard to make devenum() interface
behave similarly on different os's. perhaps there should be another
parameter to devenum() that tells which devices it should enumerate?
i.e. all or active?

thanks,
max