Hi Johan,
On Wed, Nov 13, 2013 at 02:15:37PM +0200, Johan Hedberg wrote:
> Hi Andrei,
>
> On Mon, Nov 11, 2013, Andrei Emeltchenko wrote:
> > Handle HAL socket listen call. Create RFCOMM socket and wait for events.
> > ---
> > android/socket.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 92 insertions(+), 2 deletions(-)
> >
> > diff --git a/android/socket.c b/android/socket.c
> > index c283c5f..80cb39e 100644
> > --- a/android/socket.c
> > +++ b/android/socket.c
> > @@ -27,22 +27,112 @@
> >
> > #include <glib.h>
> > #include <stdbool.h>
> > +#include <errno.h>
> >
> > #include "lib/bluetooth.h"
> > +#include "btio/btio.h"
> > #include "log.h"
> > #include "hal-msg.h"
> > #include "hal-ipc.h"
> > #include "ipc.h"
> > +#include "adapter.h"
> > #include "socket.h"
> >
> > +/* Simple list of RFCOMM server sockets */
> > +GList *rfcomm_srv_list = NULL;
> > +/* Simple list of RFCOMM accepted sockets */
> > +GList *rfcomm_accepted_list = NULL;
>
> What are these lists needed for? When/how will their items be freed?
> How about calling them "servers" and "connections"?
I will add code freeing those.
>
> > +struct rfcomm_slot {
> > + int fd;
> > + int hal_fd;
> > + int real_sock;
> > + uint32_t channel;
> > +};
>
> You should really have comments here for what each struct member is used
> for (you have this info in your cover letter but it should also be part
> of the code). Why do you keep hal_fd here? Shouldn't we close it on our
> side as soon as we've handed it over to the HAL? Why do you use uint32_t
> for the channel? RFCOMM channels are uint8_t and have a range of 1-31.
I will change it to int since we need to write int to Android framework.
>
> > +static struct rfcomm_slot *create_rfslot(int sock)
> > {
> > - DBG("Not implemented");
> > + int fds[2] = {-1, -1};
> > + struct rfcomm_slot *rfslot;
> > +
> > + if (socketpair(AF_LOCAL, SOCK_STREAM, 0, fds) < 0) {
> > + error("socketpair(): %s", strerror(errno));
> > + return NULL;
> > + }
> > +
> > + rfslot = g_malloc0(sizeof(*rfslot));
> > + rfslot->fd = fds[0];
> > + rfslot->hal_fd = fds[1];
> > + rfslot->real_sock = sock;
> > +
> > + return rfslot;
> > +}
> > +
> > +static const uint8_t UUID_PBAP[] = {
> > + 0x00, 0x00, 0x11, 0x2F, 0x00, 0x00, 0x10, 0x00,
> > + 0x80, 0x00, 0x00, 0x80, 0x5F, 0x9B, 0x34, 0xFB
> > +};
> > +#define RFCOMM_CHAN_PBAP 19
> > +
> > +static const uint8_t UUID_OPP[] = {
> > + 0x00, 0x00, 0x11, 0x05, 0x00, 0x00, 0x10, 0x00,
> > + 0x80, 0x00, 0x00, 0x80, 0x5F, 0x9B, 0x34, 0xFB
> > +};
> > +#define RFCOMM_CHAN_OPP 12
>
> A table would probably make more sense here. What about the SDP records
> that need to be registered? Also, I'd stick to the assigned numbers from
> our doc/assigned-numbers.txt.
If I use table then we need a loop here.
So shall we change default Android assigned number to ours? Some devices
assume that numbers and we would have worse interoperability.
Best regards
Andrei Emeltchenko
>
> > +static int handle_listen(void *buf)
> > +{
> > + struct hal_cmd_sock_listen *cmd = buf;
> > + const bdaddr_t *src = bt_adapter_get_address();
> > + struct rfcomm_slot *rfslot;
> > + GIOChannel *io;
> > + GError *err = NULL;
> > + int ch;
> > +
> > + DBG("");
> > +
> > + ch = get_rfcomm_chan(cmd->uuid);
> > + if (ch < 0)
> > + return -1;
> > +
> > + DBG("rfcomm channel %u", ch);
> > +
> > + rfslot = create_rfslot(-1);
> > +
> > + io = bt_io_listen(connect_cb, NULL, rfslot, NULL, &err,
> > + BT_IO_OPT_SOURCE_BDADDR, src,
> > + BT_IO_OPT_CHANNEL, ch,
> > + BT_IO_OPT_INVALID);
>
> Might make sense to pass a proper GDestroyNotify callback here so that
> once you finally implement proper freeing of data it can be done in a
> clean way.
>
> Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
