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
>>
>>> +
>>> + 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.
>>> +}
>>> +
>>> +/* 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?
--
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
