Hi Anderson
2011/9/28 Anderson Lizardo <anderson.lizardo@xxxxxxxxxxxxx>:
> Hi Santiago,
>
> On Wed, Sep 28, 2011 at 1:48 PM, Santiago Carot-Nemesio
> <sancane@xxxxxxxxx> 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.
--
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
