Hi Luiz,
> > @@ -77,6 +78,16 @@ struct bt_att {
> > bt_att_debug_func_t debug_callback;
> > bt_att_destroy_func_t debug_destroy;
> > void *debug_data;
> > +
> > + struct bt_crypto *crypto;
> > +
> > + bool valid_local_csrk;
> > + uint8_t local_csrk[16];
> > + uint32_t local_sign_cnt;
> > +
> > + bool valid_remote_csrk;
> > + uint8_t remote_csrk[16];
> > + uint32_t remote_sign_cnt;
>
> Maybe it is better to have pointers to a structs so you can free the data once it
> becomes invalid and it also removes the necessity of extra flags just to tell if the data
> is still valid since you can check if the pointer is not NULL then it must be valid.
This is good idea to remove the extra flags. However, we define the struct to do this which
would result in some problem. Now we initialize the local_sign_cnt in the bt_att_new function
and only add local_sign_cnt value when signed write command outgoing. Meanwile, we set local
CSRK in the bt_gatt_client_set_local_csrk function when there is valid CSRK provided. This two
Parameter set at different time now.
If we have pointers to a struct including local_csrk and local_signd_cnt, we initialize the local_sign_cnt
meanwhile the struct should not be NULL because of incuding it. In fact, local_csrk is not valid at this time.
If we initialize the local_signed_cnt when CSRK is valid , it would be risk to change local_signed_cnt by user
calling bt_gatt_client_set_local_csrk.
> > };
> >
> > enum att_op_type {
> > @@ -176,6 +187,8 @@ struct att_send_op {
> > bt_att_response_func_t callback;
> > bt_att_destroy_func_t destroy;
> > void *user_data;
> > +
> > + struct bt_att *att;
> > };
> >
> > static void destroy_att_send_op(void *data) @@ -277,6 +290,10 @@
> > static bool encode_pdu(struct att_send_op *op, const void *pdu,
> > uint16_t length,
> > uint16_t mtu) {
> > uint16_t pdu_len = 1;
> > + struct bt_att *att = op->att;
> > +
> > + if (op->opcode & ATT_OP_SIGNED_MASK)
> > + pdu_len += BT_ATT_SIGNATURE_LEN;
> >
> > if (length && pdu)
> > pdu_len += length;
> > @@ -293,17 +310,32 @@ static bool encode_pdu(struct att_send_op *op, const
> void *pdu,
> > if (pdu_len > 1)
> > memcpy(op->pdu + 1, pdu, length);
> >
> > + if (op->opcode & ATT_OP_SIGNED_MASK) {
> > + if (bt_crypto_sign_att(att->crypto,
> > + att->local_csrk,
> > + op->pdu,
> > + 1 + length,
> > + att->local_sign_cnt,
> > + &((uint8_t *) op->pdu)[1 +
> length]))
> > + att->local_sign_cnt++;
> > + else
> > + return false;
> > + }
> > +
>
> You can probably bail out early if you check !(op->opcode & ATT_OP_SIGNED_MASK),
> also perhaps it is a good idea to start defining structs for the PDUs to simplify the
> access.
>
If we bail out this checking early , maybe we have to need two different encode_pdu
function to implement it such as naming new encode_signed_pdu function to do this.
However, I think encoding signed_write_command pdu most difference is adding
signatgure in the end of pdu. As old stack, we can see every command has their
own encode_pdu function. In fact, most procedure is common. So I think all in one
encode_pdu function is new feature in new ATT Stack, so this implementation is
at lowest price to compatible with new ATT Stack. That is why I didn’t bail out early
this checking (op->opcode & ATT_OP_SIGNED_MASK). Following this implementation,
there is no need to create another the encode function to implement signed_write,
Just using one encode_pdu function to do this is OK.
Do you think this make sense?
Thanks
Chaojie Gu
��.n��������+%������w��{.n�����{��� �^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
