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: Wed, 25 Mar 2015 01:18:23 -0700 Message-ID: Subject: Re: [PATCH v2 4/8] core: adapter: Add parameter parsing to SetDiscoveryFilter From: Jakub Pawlowski To: Luiz Augusto von Dentz Cc: Arman Uguray , BlueZ development Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Arman, On Tue, Mar 24, 2015 at 12:53 PM, Jakub Pawlowski wrote: > On Tue, Mar 24, 2015 at 2:16 AM, Luiz Augusto von Dentz > wrote: >> 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 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