From owner-freebsd-bluetooth@FreeBSD.ORG Mon May 18 16:26:35 2009 Return-Path: Delivered-To: freebsd-bluetooth@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A91701065673 for ; Mon, 18 May 2009 16:26:35 +0000 (UTC) (envelope-from maksim.yevmenkin@gmail.com) Received: from mail-pz0-f105.google.com (mail-pz0-f105.google.com [209.85.222.105]) by mx1.freebsd.org (Postfix) with ESMTP id 758408FC17 for ; Mon, 18 May 2009 16:26:35 +0000 (UTC) (envelope-from maksim.yevmenkin@gmail.com) Received: by pzk3 with SMTP id 3so2243742pzk.3 for ; Mon, 18 May 2009 09:26:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=13MLmh6ZevMH6LWSbQ136V5QMKPJHw4XB+YMbaqwrrA=; b=uRjuuIKJyGRpqAA6sI71NGovQieoGxufzOGSQA+9uz1Mr9G81h1++dJo5S0rPjA266 WG2osgXrTfVxyG8kV9OEDYpbOB58RnK+r1ImbQkSY6LWl5eVwbaBvUDh+ZPygpDmUuuq jA4CZCYQ2WXgGFnLDttJznH3RrgULIc81INAM= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=UJd5GqQ5dc+E/zgmkNpP1B5ThFvChBxNLs3TZ9elsilUsQQPI/0Mljr12NIOLeS5lT tVUzp8GxonRNEBtkhHvPNgahyJWxR0rWkntjDP9jR79PGMF4Qby5IE8NWCjt02j4k9lg ZMr9mhWw7f0u363Au1r7cmYj4OFc2fFQrious= MIME-Version: 1.0 Received: by 10.114.93.17 with SMTP id q17mr11884995wab.38.1242663995019; Mon, 18 May 2009 09:26:35 -0700 (PDT) In-Reply-To: <1242420574.009085.2429.nullmailer@galant.ukfsn.org> References: <1242294653.314348.1748.nullmailer@galant.ukfsn.org> <1242322384.832849.4269.nullmailer@galant.ukfsn.org> <1242328962.345875.22296.nullmailer@galant.ukfsn.org> <1242335731.252131.19040.nullmailer@galant.ukfsn.org> <1242420574.009085.2429.nullmailer@galant.ukfsn.org> Date: Mon, 18 May 2009 09:26:34 -0700 Message-ID: From: Maksim Yevmenkin To: Iain Hibbert Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: freebsd-bluetooth@freebsd.org Subject: Re: libhci update X-BeenThere: freebsd-bluetooth@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Using Bluetooth in FreeBSD environments List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 May 2009 16:26:35 -0000 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