From owner-freebsd-bluetooth@FreeBSD.ORG Thu May 14 21:16:33 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 8799A106566B for ; Thu, 14 May 2009 21:16:33 +0000 (UTC) (envelope-from plunky@rya-online.net) Received: from smtp01.one2one.net (smtp01.one2one.net [149.254.200.196]) by mx1.freebsd.org (Postfix) with ESMTP id 189998FC17 for ; Thu, 14 May 2009 21:16:31 +0000 (UTC) (envelope-from plunky@rya-online.net) Received: from [127.0.0.1] (helo=localhost) by smtpbarns01 with esmtp (Exim 4.50) id 1M4iI1-0002MH-UC; Thu, 14 May 2009 21:16:30 +0000 Received: from smtpbarns01 ([127.0.0.1]) by localhost (smtpbarns01 [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 08902-01; Thu, 14 May 2009 22:16:29 +0100 (BST) Received: from [10.216.51.64] (helo=rya-online.net) by smtpbarns01 with smtp (Exim 4.50) id 1M4iHt-0002MB-5U; Thu, 14 May 2009 21:16:29 +0000 Received: (nullmailer pid 27348 invoked by uid 1000); Thu, 14 May 2009 21:15:31 -0000 Date: Thu, 14 May 2009 22:15:31 +0100 (BST) To: Maksim Yevmenkin In-Reply-To: <1242328962.345875.22296.nullmailer@galant.ukfsn.org> References: <1240311202.361300.1366.nullmailer@galant.ukfsn.org> <1240352254.082638.416.nullmailer@galant.ukfsn.org> <1240386569.369073.696.nullmailer@galant.ukfsn.org> <1242294653.314348.1748.nullmailer@galant.ukfsn.org> <1242322384.832849.4269.nullmailer@galant.ukfsn.org> <1242328962.345875.22296.nullmailer@galant.ukfsn.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Message-Id: <1242335731.252131.19040.nullmailer@galant.ukfsn.org> From: Iain Hibbert X-Virus-Scanned: by amavisd-new-20030616-p10 (Debian) at example.com X-SA-Exim-Connect-IP: 127.0.0.1 X-SA-Exim-Mail-From: plunky@rya-online.net X-SA-Exim-Scanned: No (on smtpbarns01); SAEximRunCond expanded to false 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: Thu, 14 May 2009 21:16:33 -0000 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? } + 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) @@ -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? 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? @@ -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? @@ -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? This really relates to bt_devenum() as below, are we counting 'active' devices or 'all' devices, as you ignored errors? @@ -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 while (ioctl(s, SIOCNBTINFO, &btr) != -1) { if ((btr.btr_flags & BTF_UP) == 0) continue; count++; if (cb == NULL) continue; memset(&di, 0, sizeof(di)); strlcpy(di.devname, btr.btr_name, HCI_DEVNAME_SIZE); memset(&sa, 0, sizeof(sa)); sa.bt_len = sizeof(sa); sa.bt_family = AF_BLUETOOTH; bdaddr_copy(&sa.bt_bdaddr, &btr.btr_bdaddr); if (bt_devinfo(&di) == -1 || bind(s, (struct sockaddr *)&sa, sizeof(sa)) == -1 || connect(s, (struct sockaddr *)&sa, sizeof(sa)) == -1) { close(s); return -1; } if ((*cb)(s, &di, arg) != 0) break; } thats all for now :) iain