Return-Path: Date: Mon, 13 Dec 2010 17:33:36 -0300 From: Vinicius Costa Gomes To: Brian Gix Cc: linux-bluetooth@vger.kernel.org, padovan@profusion.mobi, rshaffer@codeaurora.org Subject: Re: [PATCH 1/1] Support for reading long Characteristic Values. Message-ID: <20101213203336.GC7549@eris> References: <1292261066-28100-1-git-send-email-bgix@codeaurora.org> <1292261066-28100-3-git-send-email-bgix@codeaurora.org> <20101213183951.GB7549@eris> <4D067A6C.6080404@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <4D067A6C.6080404@codeaurora.org> Sender: linux-bluetooth-owner@vger.kernel.org List-ID: On 11:56 Mon 13 Dec, Brian Gix wrote: > Hi Vinicius, > > On 12/13/2010 10:39 AM, Vinicius Costa Gomes wrote: > > On 09:24 Mon 13 Dec, Brian Gix wrote: > >> Modify existing gatt_read_char() function support the reading of > >> attributes (specifically Characteristic Values) that are longer than > >> the MTU would otherwise allow. When a result to an ATT_OP_READ_REQ > >> is received, it will be passed to the requester as always if the total > >> result was shorter than the Default MTU (23). Any results equal to or > >> longer will cause a series of READ_BLOB requests to be made, with the > >> additional results built up until the end of the Attribute is detected. > >> The full result will then be passed to the original requester. > >> > >> The end of the Attribute is detected by either a successful result to > >> the READ_BLOB request that is shorter than the Default MTU, or by an > >> error result that indicates that a read is being attempted beyond the > >> length of the attribute, or that the Attribute wasn't "Long". > >> > >> This patch is dependant on the earlier patch: > >> 0001-Implempent-READ_BLOB-encoding-for-ATT.patch > >> > >> The packet composed conforms to the Bluetooth Core v4.0 specification. > >> > >> Brian Gix > >> bgix@codeaurora.org > >> Employee of Qualcomm Innovation Center, Inc. > >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum > > I will omit the sig in the annotation in the future. > > Also, I will examine the subsequent submissions for non-TAB ws. I need > to work on my gvim settings a bit, I think. > > >> > >> --- > >> attrib/gatt.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > >> 1 files changed, 112 insertions(+), 2 deletions(-) > >> > >> diff --git a/attrib/gatt.c b/attrib/gatt.c > >> index bca8b49..d888f1d 100644 > >> --- a/attrib/gatt.c > >> +++ b/attrib/gatt.c > >> @@ -97,15 +97,125 @@ guint gatt_read_char_by_uuid(GAttrib *attrib, uint16_t start, uint16_t end, > >> pdu, plen, func, user_data, NULL); > >> } > >> > >> +struct read_long_data { > >> + GAttrib *attrib; > >> + GAttribResultFunc func; > >> + gpointer user_data; > >> + guint8 *result_data; > >> + guint16 result_len; > > > > I would call result_data "buffer" (or something like it) and result_len "size" > > (or total). What do you think? > > Will rename to buffer and size. I often avoid "size" to avoid keyword > collisions, but I don't believe it applies here. > > > > >> + guint16 handle; > >> +}; > >> + > >> +static void read_blob_helper(guint8 status, const guint8 *res_pdu, > >> + guint16 res_len, gpointer user_data) > >> +{ > >> + struct read_long_data *long_read = user_data; > >> + uint8_t pdu[ATT_DEFAULT_MTU]; > >> + guint8 *tmp; > >> + guint16 plen; > >> + guint ret_val; > >> + > >> + if (status == ATT_ECODE_ATTR_NOT_LONG || > >> + status == ATT_ECODE_INVALID_OFFSET) { > >> + status = 0; > >> + goto done; > >> + } > >> + > >> + if (status != 0 || res_len == 1) > >> + goto done; > >> + > >> + tmp = g_try_realloc(long_read->result_data, > >> + long_read->result_len + res_len - 1); > >> + > >> + if (tmp == NULL) { > >> + status = ATT_ECODE_INSUFF_RESOURCES; > >> + goto done; > >> + } > >> + > >> + memcpy(&tmp[long_read->result_len],&res_pdu[1], res_len-1); > > > > It should be "res - 1". > > > >> + long_read->result_data = tmp; > >> + long_read->result_len += res_len-1; > > > > Same here. > > > >> + > >> + if (res_len< ATT_DEFAULT_MTU) > >> + goto done; > >> + > >> + plen = enc_read_blob_req(long_read->handle, long_read->result_len-1, > > > > And here. > > done, done, done. > > > > >> + pdu, sizeof(pdu)); > >> + ret_val = g_attrib_send(long_read->attrib, ATT_OP_READ_BLOB_REQ, pdu, > >> + plen, read_blob_helper, long_read, NULL); > >> + > >> + if (ret_val != 0) > >> + return; > >> + > >> + status = ATT_ECODE_INSUFF_RESOURCES; > >> + > >> +done: > >> + long_read->func(status, long_read->result_data, long_read->result_len, > >> + long_read->user_data); > >> + g_free(long_read->result_data); > >> + g_free(long_read); > >> +} > >> + > >> +static void read_char_helper(guint8 status, const guint8 *res_pdu, > >> + guint16 res_len, gpointer user_data) > >> +{ > >> + struct read_long_data *long_read = user_data; > >> + uint8_t pdu[ATT_DEFAULT_MTU]; > >> + guint16 plen; > >> + guint ret_val; > >> + > >> + if (status != 0 || res_len< ATT_DEFAULT_MTU) > >> + goto done; > >> + > >> + long_read->result_data = g_malloc(res_len); > >> + > >> + if (long_read->result_data == NULL) > >> + goto done; > >> + > >> + memcpy(long_read->result_data, res_pdu, res_len); > >> + long_read->result_len = res_len; > >> + > >> + plen = enc_read_blob_req(long_read->handle, res_len-1, pdu, > >> + sizeof(pdu)); > >> + ret_val = g_attrib_send(long_read->attrib, ATT_OP_READ_BLOB_REQ, > >> + pdu, plen, read_blob_helper, long_read, NULL); > > > > g_attrib_send() returns an command id (something that could be used to cancel > > the command later, if needed), so I think it would make more sense to call > > ret_val "id" or just "ret". And thinking about it, I guess that the "res_" > > prefix doesn't add much meaning to "res_pdu" and "res_len". Do you agree? > > renamed res_pdu to "rpdu", to avoid name collision with outbound local > pdu variable. > > renamed res_len to "rlen" to keep the two together, and distinct from > plen, which is also used for the outbound pdu. > > renamed ret_val to "ret". In two of the functions, it has lost the > ability to be used to cancel the GATT procedure. It can cancel the > original ATT_OP_READ_REQ opcodes, but not the subsequent > ATT_OP_READ_BLOB_REQ opcodes. > > > > >> + > >> + if (ret_val != 0) > >> + return; > >> + > >> + status = ATT_ECODE_INSUFF_RESOURCES; > >> + > >> +done: > >> + long_read->func(status, res_pdu, res_len, long_read->user_data); > > > > If g_attrib_send() fails, load_read->result_data may be leaking here. > > Is there no guarantee that the GAttribResultFunc parameter will be > invoked if there is a non-Zero return value from g_attrib_send? > > If there is paths that could result in a non-response (abnormal link > termination?) then you are correct, and I will need to rethink clean-up. The case I was referring was when g_attrib_send() cannot allocate memory for the command. > > If I am correctly reading the code, I can pass a function to the > GDestroyNotify parameter of g_attrib_send, which will alert me to the > destruction of the command, at which point I could do clean-up. Is that > path foolproof? This would also entail removing all g_free clean-ups > from the helper functions, because the destroy function is always > called, including after invoking the GAttribResultFunc function. > Yeah, this path should be foolproof. The destroy function should be called even for abnormal cases, the ResultFunc is called just when the command finishes (with a response or an error). > Also: > If we intend to use the ID of the (original) command to cancel the GATT > procedure, I propose the following: > > Compound GATT procedure such as the Read Long procedure that I am > implementing, should save the original ID returned by g_attrib_send in > the nested user_data structure such as I have done. When subsequent ATT > commands are sent as part of the same GATT procedure, the new command > shall be assigned the same ID as the original. This could be done with > a function in gattrib.c with the prototype: > > g_attrib_set_id(GAttrib *attrib, guint old_id, guint new_id); > > This would allow the upper layer to cancel the entire GATT procedure, > even if it is partially completed. > > This same methodology could then be applied to long writes, and other > compound GATT procedures. > > What do you think? Sounds good, I just don't know about messing with the commands id's, maybe having some thing like command groups. For example, adding these funtions: guint g_attrib_send_full(GAttrib *attrib, guint8 opcode, const guint8 *pdu, guint16 len, guint group, GAttribResultFunc func, gpointer user_data, GDestroyNotify notify); gboolean g_attrib_cancel_group(GAttrib *attrib, guint group); How do this look? > > > > >> + g_free(long_read); > >> +} > >> + > >> guint gatt_read_char(GAttrib *attrib, uint16_t handle, GAttribResultFunc func, > >> gpointer user_data) > >> { > >> uint8_t pdu[ATT_DEFAULT_MTU]; > >> guint16 plen; > >> + guint ret_val; > >> + struct read_long_data *long_read; > >> + > >> + long_read = g_try_new0(struct read_long_data, 1); > >> + > >> + if (long_read == NULL) > >> + return 0; > >> + > >> + long_read->attrib = attrib; > >> + long_read->func = func; > >> + long_read->user_data = user_data; > >> + long_read->handle = handle; > >> > >> plen = enc_read_req(handle, pdu, sizeof(pdu)); > >> - return g_attrib_send(attrib, ATT_OP_READ_REQ, pdu, plen, func, > >> - user_data, NULL); > >> + ret_val = g_attrib_send(attrib, ATT_OP_READ_REQ, pdu, plen, > >> + read_char_helper, long_read, NULL); > >> + > >> + if (ret_val == 0) > >> + g_free(long_read); > >> + > >> + return ret_val; > >> } > >> > >> guint gatt_write_char(GAttrib *attrib, uint16_t handle, uint8_t *value, > >> -- > >> 1.7.1 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > Cheers, > > Thanks, > > -- > Brian Gix > bgix@codeaurora.org > Employee of Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum Cheers, -- Vinicius