Hi Marcel:
On Mon, Jan 28, 2013 at 12:59 PM, Marcel Holtmann <mar...@holtmann.org> wrote:
> Hi Claudio,
>
>> This patch adds the initial SCO server socket handling. BtIO and BlueZ
>> functions should not be used, with the new Profile API the objetive is
>> to get rid of these dependencies.
>> ---
>> plugins/hfp_hf_bluez5.c | 93
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 93 insertions(+)
>>
>> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
>> index 0e496ee..398dee0 100644
>> --- a/plugins/hfp_hf_bluez5.c
>> +++ b/plugins/hfp_hf_bluez5.c
>> @@ -64,6 +64,7 @@ struct hfp {
>> static GHashTable *modem_hash = NULL;
>> static GHashTable *devices_proxies = NULL;
>> static GDBusClient *bluez = NULL;
>> +static guint sco_watch = 0;
>>
>> static void hfp_debug(const char *str, void *user_data)
>> {
>> @@ -378,6 +379,89 @@ static const GDBusMethodTable profile_methods[] = {
>> { }
>> };
>>
>> +static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>> + gpointer user_data)
>> +{
>> + struct sockaddr_sco saddr;
>> + socklen_t optlen;
>> + int sk, nsk;
>> +
>> + if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL))
>> + return FALSE;
>> +
>> + sk = g_io_channel_unix_get_fd(io);
>> +
>> + memset(&saddr, 0, sizeof(saddr));
>> + optlen = sizeof(saddr);
>> +
>> + nsk = accept(sk, (struct sockaddr *) &saddr, &optlen);
>> + if (nsk < 0)
>> + return TRUE;
>> +
>> + /* TODO: Verify if the device has a modem */
>> +
>> + return TRUE;
>> +}
>> +
>> +static GIOChannel *sco_listen(int *err)
>> +{
>
> I do not like this int *err. Why is this a separate function anyway.
> Just do the whole SCO server socket setup with accept watch setup in one
> function. There is no advantage in having two functions.
OK. I will fix it.
>
>> + struct sockaddr_sco addr;
>> + GIOChannel *io;
>> + int sk, defer_setup = 1;
>> +
>> + sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_SCO);
>> + if (sk < 0) {
>> + *err = -errno;
>> + return NULL;
>> + }
>> +
>> + /* Bind to local address */
>> + memset(&addr, 0, sizeof(addr));
>> + addr.sco_family = AF_BLUETOOTH;
>> + bt_bacpy(&addr.sco_bdaddr, BDADDR_ANY);
>> +
>> + if (bind(sk, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
>> + close(sk);
>> + *err = -errno;
>> + return NULL;
>> + }
>> +
>> + if (setsockopt(sk, SOL_BLUETOOTH, BT_DEFER_SETUP,
>> + &defer_setup, sizeof(defer_setup)) < 0) {
>> + ofono_warn("Can't enable deferred setup: %s (%d)",
>> + strerror(errno), errno);
>> + *err = -errno;
>
> This is semantically incorrect. You can not return non-NULL and an
> error.
It will be removed.
>
> Can we actually make HFP 1.6 work properly without defer setup enabled?
You can't make HFP1.6 work properly, but you need to "start" the
system to support 1.5, don't set the codec negotiation flag and export
the correct SDP record.
>
>> + }
>> +
>> + io = g_io_channel_unix_new(sk);
>> + g_io_channel_set_close_on_unref(io, TRUE);
>> + g_io_channel_set_flags(io, G_IO_FLAG_NONBLOCK, NULL);
>
> Can you just open the SOCK_NONBLOCK and SOCK_CLOEXEC as we should be
> always doing.
ok. It will be fixed.
>
>> +
>> + if (listen(sk, 5) < 0) {
>> + g_io_channel_unref(io);
>> + *err = -errno;
>> + return NULL;
>> + }
>> +
>> + return io;
>> +}
>> +
>> +static int sco_init(void)
>> +{
>> + GIOCondition cond = G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL;
>
> What is this extra declaration good for? I rather have it inside the
> call that is adding the watch.
ok. I will fix it.
IMO it makes the code more readable. The compiler will optimize it anyway.
>
>> + GIOChannel *sco_io;
>> + int err = 0;
>> +
>> + sco_io = sco_listen(&err);
>> + if (sco_io == NULL)
>> + return err;
>> +
>> + sco_watch = g_io_add_watch(sco_io, cond, sco_accept, NULL);
>> + g_io_channel_unref(sco_io);
>> +
>> + return 0;
>> +}
>> +
>> static void connect_handler(DBusConnection *conn, void *user_data)
>> {
>> DBG("Registering External Profile handler ...");
>> @@ -500,6 +584,12 @@ static int hfp_init(void)
>> if (DBUS_TYPE_UNIX_FD < 0)
>> return -EBADF;
>>
>> + err = sco_init();
>> + if (err < 0) {
>> + ofono_error("SCO: %s(%d)", strerror(-err), -err);
>> + return err;
>> + }
>> +
>> /* Registers External Profile handler */
>> if (!g_dbus_register_interface(conn, HFP_EXT_PROFILE_PATH,
>> BLUEZ_PROFILE_INTERFACE,
>> @@ -550,6 +640,9 @@ static void hfp_exit(void)
>>
>> g_hash_table_destroy(modem_hash);
>> g_hash_table_destroy(devices_proxies);
>> +
>> + if (sco_watch)
>
> Normally we do sco_watch > 0 for the GLib sources.
ok. I will fix it.
Regards,
Claudio
>
>> + g_source_remove(sco_watch);
>> }
>>
>> OFONO_PLUGIN_DEFINE(hfp_bluez5, "External Hands-Free Profile Plugin",
>> VERSION,
>
> Regards
>
> Marcel
>
>
> _______________________________________________
> ofono mailing list
> ofono@ofono.org
> http://lists.ofono.org/listinfo/ofono
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
