Hi Arman,
On Tue, Mar 24, 2015 at 12:53 PM, Jakub Pawlowski <jpawlowski@xxxxxxxxxx> wrote:
> On Tue, Mar 24, 2015 at 2:16 AM, Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
>> Hi Jakub,
>>
>> On Tue, Mar 24, 2015 at 10:01 AM, Jakub Pawlowski <jpawlowski@xxxxxxxxxx> wrote:
>>> On Mon, Mar 23, 2015 at 5:22 PM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
>>>> Hi Jakub,
>>>>
>>>>> On Mon, Mar 23, 2015 at 12:31 PM, Jakub Pawlowski <jpawlowski@xxxxxxxxxx> wrote:
>>>>> This patch adds parameter parsing, and basic internal logic checks to
>>>>> SetDiscoveryFilter method.
>>>>> ---
>>>>> src/adapter.c | 172 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 172 insertions(+)
>>>>>
>>>>> diff --git a/src/adapter.c b/src/adapter.c
>>>>> index 7c51399..f57b58c 100644
>>>>> --- a/src/adapter.c
>>>>> +++ b/src/adapter.c
>>>>> @@ -92,6 +92,8 @@
>>>>> #define SCAN_TYPE_LE ((1 << BDADDR_LE_PUBLIC) | (1 << BDADDR_LE_RANDOM))
>>>>> #define SCAN_TYPE_DUAL (SCAN_TYPE_BREDR | SCAN_TYPE_LE)
>>>>>
>>>>> +#define DISTNACE_VAL_INVALID 0x7FFF
>>>>
>>>> s/DISTNACE/DISTANCE/
>>> fixed
>>>>
>>>> Also, where does the 0x7FFF come from? Why not just 0xFFFF?
>>>>> +
>>>>> static DBusConnection *dbus_conn = NULL;
>>>>>
>>>>> static bool kernel_conn_control = false;
>>>>> @@ -145,6 +147,13 @@ struct conn_param {
>>>>> uint16_t timeout;
>>>>> };
>>>>>
>>>>> +struct discovery_filter {
>>>>> + uint8_t type;
>>>>> + uint16_t pathloss;
>>>>> + int16_t rssi;
>>>>> + GSList *uuids;
>>>>> +};
>>>>> +
>>>>> struct watch_client {
>>>>> struct btd_adapter *adapter;
>>>>> char *owner;
>>>>> @@ -1760,9 +1769,172 @@ static DBusMessage *start_discovery(DBusConnection *conn,
>>>>> return dbus_message_new_method_return(msg);
>>>>> }
>>>>>
>>>>> +static bool parse_discovery_filter_entry(char *key, DBusMessageIter *value,
>>>>> + GSList **uuids, int16_t *rssi,
>>>>> + uint16_t *pathloss, uint8_t *transport)
>>>>> +{
>>>>> + uint8_t type;
>>>>> +
>>>>> + if (strcmp("UUIDs", key) == 0) {
>>>>
>>>> Use "if (!strcmp("UUIDs"..." instead of "== 0".
>>>>
>>> fixed
>>>>> + DBusMessageIter arriter;
>>>>> +
>>>>> + if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_ARRAY)
>>>>> + return false;
>>>>
>>>> New line after return.
>>>>
>>> fixed
>>>>> + dbus_message_iter_recurse(value, &arriter);
>>>>> + while ((type = dbus_message_iter_get_arg_type(&arriter)) !=
>>>>> + DBUS_TYPE_INVALID) {
>>>>> + char *uuid_str;
>>>>> + char *result_uuid;
>>>>> + uuid_t uuid_tmp;
>>>>
>>>> I'd use bt_uuid_t here instead of uuid_t, since the latter is defined
>>>> for sdp specific usage. Though I'd check with the others here still.
>>> still discussing
So I checked that, and you're right - I should use bt_uuid_t, it's
mentioned in TODO document.
I fixed that + fixed some comment style problems
>>>>
>>>>> +
>>>>> + if (dbus_message_iter_get_arg_type(&arriter) !=
>>>>> + DBUS_TYPE_STRING)
>>>>> + return false;
>>>>> +
>>>>> + dbus_message_iter_get_basic(&arriter, &uuid_str);
>>>>> + if (bt_string2uuid(&uuid_tmp, uuid_str) < 0)
>>>>> + return false;
>>>>> +
>>>>> + result_uuid = bt_uuid2string(&uuid_tmp);
>>>>> +
>>>>> + *uuids = g_slist_prepend(*uuids, result_uuid);
>>>>> +
>>>>> + dbus_message_iter_next(&arriter);
>>>>
>>>> Indentation looks off here. You should probably run checkpatch on
>>>> these for general formatting.
>>> fixed
>>>>
>>>>> + }
>>>>> + } else if (strcmp("RSSI", key) == 0) {
>>>>> + if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_INT16)
>>>>> + return false;
>>>>
>>>> It makes code more readable if you add a new line after returns within a block.
>>>>
>>> fixed
>>>>> + dbus_message_iter_get_basic(value, rssi);
>>>>> + if (*rssi > 0 || *rssi < -127)
>>>>
>>>> Why are positive RSSI values not allowed? doc/adapter.txt doesn't
>>>> provide the unit of measurement for RSSI but I assume it's dBm, which
>>>> can be positive, no? Eitherway, add a comment here explaining these
>>>> checks.
>>>>
>>> I checked that in spec, added proper comment
>>>>> + return false;
>>>>> + } else if (strcmp("Pathloss", key) == 0) {
>>>>> + if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_UINT16)
>>>>> + return false;
>>>>> + dbus_message_iter_get_basic(value, pathloss);
>>>>> + if (*pathloss > 127)
>>>>
>>>> Add a comment here explaining why this is. Also it probably makes
>>>> sense to define a macro constant for 127.
>>> actually after looking at spec, max pathloss might be 137, RSSI =
>>> -127, TX_POWER=10
>>>>
>>>>> + return false;
>>>>> + } else if (strcmp("Transport", key) == 0) {
>>>>> + char *transport_str;
>>>>> +
>>>>> + if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_STRING)
>>>>> + return false;
>>>>> + dbus_message_iter_get_basic(value, &transport_str);
>>>>> +
>>>>> + if (strcmp(transport_str, "bredr") == 0)
>>>>> + *transport = SCAN_TYPE_BREDR;
>>>>> + else if (strcmp(transport_str, "le") == 0)
>>>>> + *transport = SCAN_TYPE_LE;
>>>>> + else if (strcmp(transport_str, "auto") == 0)
>>>>> + *transport = SCAN_TYPE_DUAL;
>>>>> + else
>>>>> + return false;
>>>>> + } else {
>>>>> + DBG("Unknown key parameter: %s!\n", key);
>>>>> + return false;
>>>>> + }
>>>>> +
>>>>> + return true;
>>
>> You could perhaps split these into e.g. parse_pathloss, parse_transport, etc.
>>
> Done. Looks better, thanks!
>>>>> +}
>>>>> +
>>>>> +/* This method is responsible for parsing parameters to SetDiscoveryFilter. If
>>>>> + * filter in msg was empty, sets *filter to NULL. If whole parsing was
>>>>> + * successful, sets *filter to proper value.
>>>>> + * Returns false on any error, and true on success.
>>>>> + */
>>>>> +static bool parse_discovery_filter_dict(struct discovery_filter **filter,
>>>>> + DBusMessage *msg)
>>
>> Why don't you return the struct discovery_filter * instead of bool?
>
> Returned value means that parsing was successful/unsuccessful.
> When it was successfull, the filter parameter will be either NULL
> (filter was cleared), or point to the filter.
> I think it's according to convention (returning status). How would you
> like that modified?
>
>>
>>
>> --
>> Luiz Augusto von Dentz
--
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
