Return-Path: <cyrus@holtmann.org>
MIME-Version: 1.0
In-Reply-To: <CAJdJm_OGGROUJ1NoJAdT09CTVEngrNdfxYWC5c7kp8RhJcvvcA@mail.gmail.com>
References: <1317232126-12640-1-git-send-email-sancane@gmail.com>
	<1317232126-12640-2-git-send-email-sancane@gmail.com>
	<1317232126-12640-3-git-send-email-sancane@gmail.com>
	<1317232126-12640-4-git-send-email-sancane@gmail.com>
	<1317232126-12640-5-git-send-email-sancane@gmail.com>
	<1317232126-12640-6-git-send-email-sancane@gmail.com>
	<1317232126-12640-7-git-send-email-sancane@gmail.com>
	<1317232126-12640-8-git-send-email-sancane@gmail.com>
	<CAJdJm_OGGROUJ1NoJAdT09CTVEngrNdfxYWC5c7kp8RhJcvvcA@mail.gmail.com>
Date: Thu, 29 Sep 2011 15:41:40 +0200
Message-ID: <CACLukzLcc-SOiVrcZ1wuxww=PqjB9g-x7oZ1U+UH3=GgtyFP=w@mail.gmail.com>
Subject: Re: [PATCH 7/9] Read temperature type characteristic
From: Santiago Carot <sancane@gmail.com>
To: Anderson Lizardo <anderson.lizardo@openbossa.org>
Cc: linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset=ISO-8859-1
Sender: linux-bluetooth-owner@vger.kernel.org
List-ID: <linux-bluetooth.vger.kernel.org>

Hi Anderson

2011/9/28 Anderson Lizardo <anderson.lizardo@openbossa.org>:
> Hi Santiago,
>
> On Wed, Sep 28, 2011 at 1:48 PM, Santiago Carot-Nemesio
> <sancane@gmail.com> wrote:
>> +static gchar* temptype2str(uint8_t value)
>
> Small conding style issue here: gchar* ?-> gchar *
>
>> +{
>> + ? ? ? switch (value) {
>> + ? ? ? case 1: return "Armpit";
>> + ? ? ? case 2: return "Body";
>> + ? ? ? case 3: return "Ear";
>> + ? ? ? case 4: return "Finger";
>> + ? ? ? case 5: return "Intestines";
>> + ? ? ? case 6: return "Mouth";
>> + ? ? ? case 7: return "Rectum";
>> + ? ? ? case 8: return "Toe";
>> + ? ? ? case 9: return "Tympanum";
>> + ? ? ? default:
>> + ? ? ? ? ? ? ? error("Temperature type %d reserved for future use", value);
>> + ? ? ? ? ? ? ? return NULL;
>> + ? ? ? };
>> +}
>> +
>> ?static void destroy_char(gpointer user_data)
>> ?{
>> ? ? ? ?struct characteristic *c = user_data;
>> @@ -113,7 +138,27 @@ static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
>> ?static void read_temp_type_cb(guint8 status, const guint8 *pdu, guint16 len,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?gpointer user_data)
>> ?{
>> - ? ? ? /* TODO */
>> + ? ? ? struct characteristic *ch = user_data;
>> + ? ? ? struct thermometer *t = ch->t;
>> + ? ? ? uint8_t value;
>> + ? ? ? int vlen;
>> +
>> + ? ? ? if (status != 0) {
>> + ? ? ? ? ? ? ? DBG("Temperature Type value read failed: %s",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? att_ecode2str(status));
>> + ? ? ? ? ? ? ? return;
>> + ? ? ? }
>> +
>> + ? ? ? if (!dec_read_resp(pdu, len, &value, &vlen)) {
>
> Your code is assuming that the read response PDU will always have a
> one-byte value. This will cause memory corruption if the peer sends an
> error response with size greater than 2 (first byte is the opcode).
>
> Either put a check like "if (len == sizeof(uint8_t) + 1 &&
> read_read_resp(...))", or use:
>
> uint8_t value[ATT_MAX_MTU];
>
> and cast the first byte.
>

You're right. I'm fixing it. Gatt API is quite confusing.

> (just noticed that Proximity monitor code has the same bug...)
>
>> + ? ? ? ? ? ? ? DBG("Protocol error");
>> + ? ? ? ? ? ? ? return;
>> + ? ? ? }
>> +
>> + ? ? ? DBG("TEMPERATURE TYPE: %s", temptype2str(value));
>
> Can you avoid using all-caps on the DBG() message above?
>
>> + ? ? ? if (!t->has_interval)
>> + ? ? ? ? ? ? ? t->has_interval = TRUE;
>
> Why the if() check? You could just do "t->has_interval = TRUE;" always.
>

Ok.
I'm going to send a new set of patches fixing this issues.

Regards.