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.
>
> Well I guess this was your design, it doesn't have to be this way, in fact I believe
> there is no point of initializing the counter if there is no valid CSRK because one
> depend on the other, also perhaps you have a bug in bt_gatt_client_set_local_csrk
> since I believe every time you set CSRK you should reset the counter otherwise you
> may be carrying the counters from the past CSRK which probably won't work.
You said condition resuilt in bug which I want to avoid, because we use this command line
By btgatt-client tool
Write-value -w -c <CSRK> <value_handle> <value>
If user write signed command twice, it would call bt_gatt_client_set_local_csrk twice.
So it will restore sign_cnt, that's why I initialize the sign_cnt in bt_att_new to avoid this bug.
In fact, the only way is that we should tell user set CSRK once if CSRK did not change to
resolve this problem.
> Also don't to have to pass the counter when setting the key, the counter probably
> need to be restored on every connection so bt_gatt_client_set_local_csrk and
> bt_att_set_remote_csrk probably needs to take the counter and instead of having a
> valid flag you can probably reset by setting NULL as key or an invalid counter if one
> exists.
If we define the new struct naming signed_info to store csrk and sign_cnt, what's your point?
>
> > 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.
>
> But you return true anyway after this code, no idea why you need a new function for
> that, all I was suggesting is the following:
>
> if (!(op->opcode & ATT_OP_SIGNED_MASK))
> return true;
>
> if (!bt_crypto_sign_att...)
> return false;
>
> att->local_sign_cnt++;
>
> return true;
>
I got your meaning now. I have returned false when bt_crypto_sign_att. Failed
+ else
+ return false;
Your proposal is good so as to make code more clearer. I would accept that.
Thanks
Chaojie Gu
��.n��������+%������w��{.n�����{��� �^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
