Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1427139103-26441-1-git-send-email-jpawlowski@google.com> <1427139103-26441-4-git-send-email-jpawlowski@google.com> Date: Tue, 24 Mar 2015 11:16:09 +0200 Message-ID: Subject: Re: [PATCH v2 4/8] core: adapter: Add parameter parsing to SetDiscoveryFilter From: Luiz Augusto von Dentz To: Jakub Pawlowski Cc: Arman Uguray , BlueZ development Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Jakub, On Tue, Mar 24, 2015 at 10:01 AM, Jakub Pawlowski wrote: > On Mon, Mar 23, 2015 at 5:22 PM, Arman Uguray wrote: >> Hi Jakub, >> >>> On Mon, Mar 23, 2015 at 12:31 PM, Jakub Pawlowski 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