Re: [PATCH v1 7/8] hfp_hf_bluez5: Add rejecting SCO connection

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6884 bytes --]

Hi Claudio,

>>> This patch rejects the SCO incoming connection if there isn't a modem
>>> (service level connection) associated with the address.
>>
>>
>> Why service level connection is in parenthesis?
>
> I thought this additional information could be useful.
> No problem, I gonna remove it.
>

Actually the service level connection is important, the modem part is 
not.  Hence why I'm lost why it is in parenthesis.

<snip>

>>> +       bdaddr_t src;
>>> +       bdaddr_t dst;
>>> +};
>>
>>
>> What is src and what is dst? This is completely context dependent and
>> confusing.  Also, we are already storing the device address on the modem
>> object, why do we need this yet again?
>
> We use this standard in BlueZ for source and destination. It could be
> also sba and dba.
> Do you have another suggestion?

Then BlueZ is just plain weird.  Local and Remote would be way better names.

>
> Indeed, we already have the address stored in the modem, but it is the
> opposite thing of what Marcel is asking: avoid BT address stored using
> string format.
> If I use string to store the BT source address I will be blamed also,
> I did this for the PATCH v0. Please define how you want to store this
> data.
>

Whenever we store duplicate information, a question will be raised.  I 
see no point in doing it twice.

>>
>>
>>> +
>>> +struct sock_bdaddr {
>>> +       bdaddr_t src;
>>> +       bdaddr_t dst;
>>>    };
>>
>>
>> I'd really like to avoid this structure.  At the very least change the name
>> into something that conveys its purpose.  e.g. bdaddr_pair, or something.
>> The way it is now is just silly.
>
> If we get the Device address from the modem string this struct will
> not be necessary.
>
>>
>>>
>>>    static GHashTable *modem_hash = NULL;
>>> @@ -68,6 +75,21 @@ static GHashTable *devices_proxies = NULL;
>>>    static GDBusClient *bluez = NULL;
>>>    static guint sco_watch = 0;
>>>
>>> +static gboolean modem_bacmp(gpointer key, gpointer value,
>>> +                                               gpointer user_data)
>>> +{
>>> +       const struct sock_bdaddr *bda = user_data;
>>> +       struct ofono_modem *modem = value;
>>> +       struct hfp *hfp = ofono_modem_get_data(modem);
>>> +       int ret;
>>> +
>>> +       ret = bt_bacmp(&bda->dst,&hfp->dst);
>>>
>>> +       if (ret != 0)
>>> +               return FALSE;
>>> +
>>> +       return bt_bacmp(&bda->src,&hfp->src) != 0 ? FALSE : TRUE;
>>>
>>> +}
>>> +
>>>    static void hfp_debug(const char *str, void *user_data)
>>>    {
>>>          const char *prefix = user_data;
>>> @@ -275,14 +297,15 @@ static DBusMessage
>>> *profile_new_connection(DBusConnection *conn,
>>>    {
>>>          struct hfp *hfp;
>>>          struct ofono_modem *modem;
>>> +       struct sockaddr_rc src, dst;
>>> +       socklen_t alen;
>>>          DBusMessageIter iter;
>>>          GDBusProxy *proxy;
>>>          DBusMessageIter entry;
>>> -       const char *device, *alias, *address;
>>> +       const char *device, *alias;
>>> +       char device_address[18];
>>>          int fd, err;
>>>
>>> -       DBG("Profile handler NewConnection");
>>> -
>>>          if (dbus_message_iter_init(msg,&entry) == FALSE)
>>>
>>>                  goto invalid;
>>>
>>> @@ -301,11 +324,6 @@ static DBusMessage
>>> *profile_new_connection(DBusConnection *conn,
>>>
>>>          dbus_message_iter_get_basic(&iter,&alias);
>>>
>>> -       if (g_dbus_proxy_get_property(proxy, "Address",&iter) == FALSE)
>>>
>>> -               goto invalid;
>>> -
>>> -       dbus_message_iter_get_basic(&iter,&address);
>>> -
>>>          dbus_message_iter_next(&entry);
>>>          if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_UNIX_FD)
>>>                  goto invalid;
>>> @@ -314,7 +332,24 @@ static DBusMessage
>>> *profile_new_connection(DBusConnection *conn,
>>>          if (fd<   0)
>>>                  goto invalid;
>>>
>>> -       modem = modem_register(device, address, alias);
>>> +       memset(&src, 0, sizeof(src));
>>> +       alen = sizeof(src);
>>> +       if (getsockname(fd, (struct sockaddr *)&src,&alen)<   0) {
>>>
>>> +               close(fd);
>>> +               goto invalid;
>>> +       }
>>> +
>>> +       memset(&dst, 0, sizeof(dst));
>>> +       alen = sizeof(dst);
>>> +       if (getpeername(fd, (struct sockaddr *)&dst,&alen)<   0) {
>>>
>>> +               close(fd);
>>> +               goto invalid;
>>> +       }
>>> +
>>> +       bt_ba2str(&dst.rc_bdaddr, device_address);
>>> +       DBG("Profile handler NewConnection: %s", device_address);
>>> +
>>> +       modem = modem_register(device, device_address, alias);
>>>          if (modem == NULL) {
>>>                  close(fd);
>>>                  return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
>>> @@ -329,6 +364,8 @@ static DBusMessage
>>> *profile_new_connection(DBusConnection *conn,
>>>                                          "Not enough resources");
>>>
>>>          hfp = ofono_modem_get_data(modem);
>>> +       bt_bacpy(&hfp->src,&src.rc_bdaddr);
>>> +       bt_bacpy(&hfp->dst,&dst.rc_bdaddr);
>>>          hfp->msg = dbus_message_ref(msg);
>>>
>>
>> Fair enough, but this chunk really belongs in a separate patch.
>
> IMO it belongs to this patch since it is initializing a value that
> will be used to compare the addresses when the sco connection is
> accepted.
> But no problem, I can split it.
>

I'd like to see the profile_new_connection changes as a separate patch, 
this part can be there as well, not a biggie.

>>
>>
>>>          return NULL;
>>> @@ -384,7 +421,9 @@ static const GDBusMethodTable profile_methods[] = {
>>>    static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>>>                                                          gpointer
>>> user_data)
>>>    {
>>> -       struct sockaddr_sco saddr;
>>> +       struct ofono_modem *modem;
>>> +       struct sockaddr_sco src, dst;
>>> +       struct sock_bdaddr bda;
>>>          socklen_t alen;
>>>          int sk, nsk;
>>>
>>> @@ -393,14 +432,29 @@ static gboolean sco_accept(GIOChannel *io,
>>> GIOCondition cond,
>>>
>>>          sk = g_io_channel_unix_get_fd(io);
>>>
>>> -       memset(&saddr, 0, sizeof(saddr));
>>> -       alen = sizeof(saddr);
>>> +       memset(&dst, 0, sizeof(dst));
>>> +       alen = sizeof(dst);
>>>
>>> -       nsk = accept(sk, (struct sockaddr *)&saddr,&alen);
>>> +       nsk = accept(sk, (struct sockaddr *)&dst,&alen);
>>
>>
>> When reading this patch I go 'what the...'
>
> I am renaming it to follow a standard: src for source and dst for destination.
> But I can revert it, no problem.
>

This is a dual-initiator profile, src and dst are really bad names for this.

Regards,
-Denis