Re: [PATCH] android/hidhost: Fix hex string to buffer convertion

Re: [PATCH] android/hidhost: Fix hex string to buffer convertion

[Luiz Augusto von Dentz]

Hi,

On Wed, Apr 9, 2014 at 11:49 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Szymon,
>
>> Due to missing limit specifier buffer was always filled with last hex
>> value in string.
>> ---
>> android/hidhost.c | 23 ++++++++++++++---------
>> 1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/android/hidhost.c b/android/hidhost.c
>> index 5ea7c5a..124d710 100644
>> --- a/android/hidhost.c
>> +++ b/android/hidhost.c
>> @@ -162,10 +162,18 @@ static void hid_device_remove(struct hid_device *dev)
>>       hid_device_free(dev);
>> }
>>
>> +static void hex2buf(const uint8_t *hex, uint8_t *buf, int num)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < num; i++)
>> +             sscanf((const char *)(hex + (i * 2)), "%02hhX", &buf[i]);
>> +}
>
> can we please build a cheaper version of this that does not require to use sscanf. Small hint is to look into src/util.c from oFono.

I also wonder why we are doing this? All the HAL does is to
memcpy(cmd->data, data, cmd->len) or is it because the string itself
is hexadecimal encoded? If that is the case someone probably deserves
a medal...


-- 
Luiz Augusto von Dentz

Re: [PATCH] android/hidhost: Fix hex string to buffer convertion

[Szymon Janc]

Hi Luiz,

On Thursday 10 of April 2014 12:57:00 Luiz Augusto von Dentz wrote:
> Hi,
> 
> On Wed, Apr 9, 2014 at 11:49 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> > Hi Szymon,
> >
> >> Due to missing limit specifier buffer was always filled with last hex
> >> value in string.
> >> ---
> >> android/hidhost.c | 23 ++++++++++++++---------
> >> 1 file changed, 14 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/android/hidhost.c b/android/hidhost.c
> >> index 5ea7c5a..124d710 100644
> >> --- a/android/hidhost.c
> >> +++ b/android/hidhost.c
> >> @@ -162,10 +162,18 @@ static void hid_device_remove(struct hid_device *dev)
> >>       hid_device_free(dev);
> >> }
> >>
> >> +static void hex2buf(const uint8_t *hex, uint8_t *buf, int num)
> >> +{
> >> +     int i;
> >> +
> >> +     for (i = 0; i < num; i++)
> >> +             sscanf((const char *)(hex + (i * 2)), "%02hhX", &buf[i]);
> >> +}
> >
> > can we please build a cheaper version of this that does not require to use sscanf. Small hint is to look into src/util.c from oFono.
> 
> I also wonder why we are doing this? All the HAL does is to
> memcpy(cmd->data, data, cmd->len) or is it because the string itself
> is hexadecimal encoded? If that is the case someone probably deserves
> a medal...

Yeap, Java is passing string to send_data() and set_report() calls (those have no
size parameter).

In JNI you can see:
 const char *c_report = env->GetStringUTFChars(report, NULL);
    if ( (status = sBluetoothHidInterface->send_data((bt_bdaddr_t *) addr, (char*) c_report)) !=
             BT_STATUS_SUCCESS) {

Those are test commands so not that important. But I wonder why this is done
also in handle_uhid_output()...   Ravi, could you comment on that?

-- 
Best regards, 
Szymon Janc

Re: [PATCH] android/hidhost: Fix hex string to buffer convertion

[Ravi kumar Veeramally]

Hi Szymon,

On 04/10/2014 01:32 PM, Szymon Janc wrote:
> Hi Luiz,
>
> On Thursday 10 of April 2014 12:57:00 Luiz Augusto von Dentz wrote:
>> Hi,
>>
>> On Wed, Apr 9, 2014 at 11:49 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>>> Hi Szymon,
>>>
>>>> Due to missing limit specifier buffer was always filled with last hex
>>>> value in string.
>>>> ---
>>>> android/hidhost.c | 23 ++++++++++++++---------
>>>> 1 file changed, 14 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/android/hidhost.c b/android/hidhost.c
>>>> index 5ea7c5a..124d710 100644
>>>> --- a/android/hidhost.c
>>>> +++ b/android/hidhost.c
>>>> @@ -162,10 +162,18 @@ static void hid_device_remove(struct hid_device *dev)
>>>>        hid_device_free(dev);
>>>> }
>>>>
>>>> +static void hex2buf(const uint8_t *hex, uint8_t *buf, int num)
>>>> +{
>>>> +     int i;
>>>> +
>>>> +     for (i = 0; i < num; i++)
>>>> +             sscanf((const char *)(hex + (i * 2)), "%02hhX", &buf[i]);
>>>> +}
>>> can we please build a cheaper version of this that does not require to use sscanf. Small hint is to look into src/util.c from oFono.
>> I also wonder why we are doing this? All the HAL does is to
>> memcpy(cmd->data, data, cmd->len) or is it because the string itself
>> is hexadecimal encoded? If that is the case someone probably deserves
>> a medal...
> Yeap, Java is passing string to send_data() and set_report() calls (those have no
> size parameter).
>
> In JNI you can see:
>   const char *c_report = env->GetStringUTFChars(report, NULL);
>      if ( (status = sBluetoothHidInterface->send_data((bt_bdaddr_t *) addr, (char*) c_report)) !=
>               BT_STATUS_SUCCESS) {
>
> Those are test commands so not that important. But I wonder why this is done
> also in handle_uhid_output()...   Ravi, could you comment on that?
   Could not recall exactly, probably done similar to set_report due to
   req[0] = HID_MSG_SET_REPORT | output->rtype;

Regards,
Ravi.