[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r122381160 ## File path: net/nimble/host/include/host/ble_store.h ## @@ -131,6 +137,40 @@ union ble_store_value { struct ble_store_value_cccd cccd; }; +struct ble_store_status_event { +/** + * The type of object that failed to persist; one of the + * BLE_STORE_OBJ_TYPE_[...] codes. + */ +int obj_type; + +/** + * The type of event being reported; one of the BLE_STORE_EVENT_TYPE_[...] + * codes. + */ +int event_code; + +/** + * Additional data related to the event; the valid field is inferred from + * the obj_type,event_code pair. + */ +union { +/** + * The record that failed to be written. Valid for the following event + * types: + * o BLE_STORE_EVENT_OVERFLOW Review comment: Ok, please see below what I have in mind. ``` union { /** * The record that failed to be written. Valid for all the obj_type and following event * types: * o BLE_STORE_EVENT_OVERFLOW */ const union ble_store_value *value; /** * The connection handle of the peer. Valid for the following event * types: * o BLE_STORE_EVENT_OVERFLOW_NEXT * * and following obj_types: * * o BLE_STORE_OBJ_TYPE_OUR_SEC * o BLE_STORE_OBJ_TYPE_PEER_SEC */ uint16_t conn_handle; }; ``` In this way, whenever we need to handle object which also uses BLE_STORE_EVENT_OVERFLOW_NEXT but has different data than conn_handle, we can easly add it here. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r122381160 ## File path: net/nimble/host/include/host/ble_store.h ## @@ -131,6 +137,40 @@ union ble_store_value { struct ble_store_value_cccd cccd; }; +struct ble_store_status_event { +/** + * The type of object that failed to persist; one of the + * BLE_STORE_OBJ_TYPE_[...] codes. + */ +int obj_type; + +/** + * The type of event being reported; one of the BLE_STORE_EVENT_TYPE_[...] + * codes. + */ +int event_code; + +/** + * Additional data related to the event; the valid field is inferred from + * the obj_type,event_code pair. + */ +union { +/** + * The record that failed to be written. Valid for the following event + * types: + * o BLE_STORE_EVENT_OVERFLOW Review comment: Ok, please see below what I have in mind. union { /** * The record that failed to be written. Valid for all the obj_type and following event * types: * o BLE_STORE_EVENT_OVERFLOW */ const union ble_store_value *value; /** * The connection handle of the peer. Valid for the following event * types: * o BLE_STORE_EVENT_OVERFLOW_NEXT * * and following obj_types: * * o BLE_STORE_OBJ_TYPE_OUR_SEC * o BLE_STORE_OBJ_TYPE_PEER_SEC */ uint16_t conn_handle; }; In this way, whenever we need to handle object which also uses BLE_STORE_EVENT_OVERFLOW_NEXT but has different data than conn_handle, we can easly add it here. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r122381160 ## File path: net/nimble/host/include/host/ble_store.h ## @@ -131,6 +137,40 @@ union ble_store_value { struct ble_store_value_cccd cccd; }; +struct ble_store_status_event { +/** + * The type of object that failed to persist; one of the + * BLE_STORE_OBJ_TYPE_[...] codes. + */ +int obj_type; + +/** + * The type of event being reported; one of the BLE_STORE_EVENT_TYPE_[...] + * codes. + */ +int event_code; + +/** + * Additional data related to the event; the valid field is inferred from + * the obj_type,event_code pair. + */ +union { +/** + * The record that failed to be written. Valid for the following event + * types: + * o BLE_STORE_EVENT_OVERFLOW Review comment: Ok, please see below what I have in mind. union { /** * The record that failed to be written. Valid for all the obj_type and following event * types: * o BLE_STORE_EVENT_OVERFLOW */ const union ble_store_value *value; /** * The connection handle of the peer. Valid for the following event * types: * o BLE_STORE_EVENT_OVERFLOW_NEXT * * and following obj_types: * * o BLE_STORE_OBJ_TYPE_OUR_SEC * o BLE_STORE_OBJ_TYPE_PEER_SEC */ uint16_t conn_handle; }; In this way, whenever we need to handle object which also uses BLE_STORE_EVENT_OVERFLOW_NEXT but has different data than conn_handle, we can easly add it here. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r122190062 ## File path: net/nimble/host/src/ble_sm.c ## @@ -892,6 +895,72 @@ ble_sm_key_dist(struct ble_sm_proc *proc, } } +static int +ble_sm_chk_store_overflow_once(int is_our_sec, uint16_t conn_handle) +{ +#if !MYNEWT_VAL(BLE_SM_BONDING) +return 0; +#endif + +struct ble_store_status_event event; +int obj_type; +int count; +int rc; + +if (is_our_sec) { +obj_type = BLE_STORE_OBJ_TYPE_OUR_SEC; +} else { +obj_type = BLE_STORE_OBJ_TYPE_PEER_SEC; +} + +rc = ble_store_util_count(obj_type, ); +if (rc != 0) { +return rc; +} + +/* Pessimistically assume all active procs will persist bonds. */ +ble_hs_lock(); +count += ble_sm_num_procs(); +ble_hs_unlock(); + +if (count < MYNEWT_VAL(BLE_STORE_MAX_BONDS)) { +/* There is sufficient capacity for another bond. */ +return 0; +} + +/* No capacity for an additional bond. Tell the application to make + * room. + */ +memset(, 0, sizeof event); +event.obj_type = obj_type; Review comment: yes, function which take parameter. I think I mentioned before something like that: ble_store_overflow_event(obj_type, event_code, void *event_data) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r122189440 ## File path: net/nimble/host/include/host/ble_store.h ## @@ -131,6 +137,40 @@ union ble_store_value { struct ble_store_value_cccd cccd; }; +struct ble_store_status_event { +/** + * The type of object that failed to persist; one of the + * BLE_STORE_OBJ_TYPE_[...] codes. + */ +int obj_type; + +/** + * The type of event being reported; one of the BLE_STORE_EVENT_TYPE_[...] + * codes. + */ +int event_code; + +/** + * Additional data related to the event; the valid field is inferred from + * the obj_type,event_code pair. + */ +union { +/** + * The record that failed to be written. Valid for the following event + * types: + * o BLE_STORE_EVENT_OVERFLOW Review comment: yes it is it This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r121058416 ## File path: net/nimble/host/src/ble_sm.c ## @@ -892,6 +895,72 @@ ble_sm_key_dist(struct ble_sm_proc *proc, } } +static int +ble_sm_chk_store_overflow_once(int is_our_sec, uint16_t conn_handle) +{ +#if !MYNEWT_VAL(BLE_SM_BONDING) +return 0; +#endif + +struct ble_store_status_event event; +int obj_type; +int count; +int rc; + +if (is_our_sec) { +obj_type = BLE_STORE_OBJ_TYPE_OUR_SEC; +} else { +obj_type = BLE_STORE_OBJ_TYPE_PEER_SEC; +} + +rc = ble_store_util_count(obj_type, ); +if (rc != 0) { +return rc; +} + +/* Pessimistically assume all active procs will persist bonds. */ +ble_hs_lock(); +count += ble_sm_num_procs(); +ble_hs_unlock(); + +if (count < MYNEWT_VAL(BLE_STORE_MAX_BONDS)) { +/* There is sufficient capacity for another bond. */ +return 0; +} + +/* No capacity for an additional bond. Tell the application to make + * room. + */ +memset(, 0, sizeof event); +event.obj_type = obj_type; Review comment: I was hoping to extract that part of code as a helper which sends overflow event, but maybe we can do it when see need to construct same events in more places. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r121056646 ## File path: net/nimble/host/include/host/ble_store.h ## @@ -131,6 +137,40 @@ union ble_store_value { struct ble_store_value_cccd cccd; }; +struct ble_store_status_event { +/** + * The type of object that failed to persist; one of the + * BLE_STORE_OBJ_TYPE_[...] codes. + */ +int obj_type; + +/** + * The type of event being reported; one of the BLE_STORE_EVENT_TYPE_[...] + * codes. + */ +int event_code; + +/** + * Additional data related to the event; the valid field is inferred from + * the obj_type,event_code pair. + */ +union { +/** + * The record that failed to be written. Valid for the following event + * types: + * o BLE_STORE_EVENT_OVERFLOW Review comment: Since a key for union data is pair (obj_type, event_code pair) could we extend comment here and say that value is valid for event type "BLE_STORE_EVENT_OVERFLOW" and all possible obj_type. Similar we could do for description to conn_handle, but int this case we have only couple of obj_types valid. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r121058053 ## File path: net/nimble/host/include/host/ble_store.h ## @@ -31,6 +31,12 @@ extern "C" { #define BLE_STORE_OBJ_TYPE_PEER_SEC 2 #define BLE_STORE_OBJ_TYPE_CCCD 3 +/** Failed to persist record; insufficient storage capacity. */ +#define BLE_STORE_EVENT_OVERFLOW1 + +/** About to execute a procedure that may fail due to overflow. */ +#define BLE_STORE_EVENT_OVERFLOW_NEXT 2 Review comment: Well I don't have very strong opinion on this, so I suggest to leave it as it is. In the end I think that my confusion was created due to comments on the fields in union. Please check comment over there. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r121058416 ## File path: net/nimble/host/src/ble_sm.c ## @@ -892,6 +895,72 @@ ble_sm_key_dist(struct ble_sm_proc *proc, } } +static int +ble_sm_chk_store_overflow_once(int is_our_sec, uint16_t conn_handle) +{ +#if !MYNEWT_VAL(BLE_SM_BONDING) +return 0; +#endif + +struct ble_store_status_event event; +int obj_type; +int count; +int rc; + +if (is_our_sec) { +obj_type = BLE_STORE_OBJ_TYPE_OUR_SEC; +} else { +obj_type = BLE_STORE_OBJ_TYPE_PEER_SEC; +} + +rc = ble_store_util_count(obj_type, ); +if (rc != 0) { +return rc; +} + +/* Pessimistically assume all active procs will persist bonds. */ +ble_hs_lock(); +count += ble_sm_num_procs(); +ble_hs_unlock(); + +if (count < MYNEWT_VAL(BLE_STORE_MAX_BONDS)) { +/* There is sufficient capacity for another bond. */ +return 0; +} + +/* No capacity for an additional bond. Tell the application to make + * room. + */ +memset(, 0, sizeof event); +event.obj_type = obj_type; Review comment: I was hoping to extract that part of code as a helper which sends overflow event, but maybe we can do it when see need to construct same events in more places. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r119708778 ## File path: net/nimble/host/include/host/ble_store.h ## @@ -31,6 +31,12 @@ extern "C" { #define BLE_STORE_OBJ_TYPE_PEER_SEC 2 #define BLE_STORE_OBJ_TYPE_CCCD 3 +/** Failed to persist record; insufficient storage capacity. */ +#define BLE_STORE_EVENT_OVERFLOW1 + +/** About to execute a procedure that may fail due to overflow. */ +#define BLE_STORE_EVENT_OVERFLOW_NEXT 2 Review comment: good point. somehow I forgot about that obj_type. So callback first checks event_type then looks into obj_type and then specific data. You are right.. in this case it makes sense to have generic event_type. So maybe we can try to improve name: e.g. BLE_STORE_EVENT_OVERFLOW_WARNING, BLE_STORE_EVENT_OVERFLOW_IS_COMING, BLE_STORE_EVENT_OVERFLOW_AHEAD, BLE_STORE_EVENT_POSSIBLE_OVERFLOW ? Not sure if any of this is better :) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r119484188 ## File path: net/nimble/host/src/ble_sm.c ## @@ -892,6 +895,72 @@ ble_sm_key_dist(struct ble_sm_proc *proc, } } +static int +ble_sm_chk_store_overflow_once(int is_our_sec, uint16_t conn_handle) +{ +#if !MYNEWT_VAL(BLE_SM_BONDING) +return 0; +#endif + +struct ble_store_status_event event; +int obj_type; +int count; +int rc; + +if (is_our_sec) { +obj_type = BLE_STORE_OBJ_TYPE_OUR_SEC; +} else { +obj_type = BLE_STORE_OBJ_TYPE_PEER_SEC; +} + +rc = ble_store_util_count(obj_type, ); +if (rc != 0) { +return rc; +} + +/* Pessimistically assume all active procs will persist bonds. */ +ble_hs_lock(); +count += ble_sm_num_procs(); Review comment: ah I see This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r119286680 ## File path: net/nimble/host/src/ble_sm.c ## @@ -1610,6 +1693,7 @@ ble_sm_pair_rsp_rx(uint16_t conn_handle, struct os_mbuf **om, struct ble_sm_pair_cmd *rsp; struct ble_sm_proc *proc; uint8_t ioact; +int rc; Review comment: This seems to be not related to this patch This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r119285543 ## File path: net/nimble/host/include/host/ble_store.h ## @@ -31,6 +31,12 @@ extern "C" { #define BLE_STORE_OBJ_TYPE_PEER_SEC 2 #define BLE_STORE_OBJ_TYPE_CCCD 3 +/** Failed to persist record; insufficient storage capacity. */ +#define BLE_STORE_EVENT_OVERFLOW1 + +/** About to execute a procedure that may fail due to overflow. */ +#define BLE_STORE_EVENT_OVERFLOW_NEXT 2 Review comment: since for BLE_STORE_EVENT_OVERFLOW_NEXT we specify conn_handle because it is used during pairing, why not to name type BLE_STORE_EVENT_OVERFLOW_BONDING or similar? That would make clear for application what is going wrong and what to do i.e. remove old bonded devices. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r119286487 ## File path: net/nimble/host/src/ble_sm.c ## @@ -892,6 +895,72 @@ ble_sm_key_dist(struct ble_sm_proc *proc, } } +static int +ble_sm_chk_store_overflow_once(int is_our_sec, uint16_t conn_handle) Review comment: maybe instead of is_our_sec we could provide obj_type? But actually maybe it would be better to create helper ble_store_overflow_event(obj_type, event_code, void *event_data) and keep this function to basically check if there is a sufficient place. I see that helper could be used at least in three places in this code. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r119287662 ## File path: net/nimble/host/src/ble_store.c ## @@ -28,49 +28,98 @@ ble_store_read(int obj_type, const union ble_store_key *key, { int rc; +ble_hs_lock(); Review comment: Is this related to this patch? BTW why we need it in ble_store? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r119285625 ## File path: net/nimble/host/src/ble_sm.c ## @@ -892,6 +895,72 @@ ble_sm_key_dist(struct ble_sm_proc *proc, } } +static int +ble_sm_chk_store_overflow_once(int is_our_sec, uint16_t conn_handle) +{ +#if !MYNEWT_VAL(BLE_SM_BONDING) +return 0; +#endif + +struct ble_store_status_event event; +int obj_type; +int count; +int rc; + +if (is_our_sec) { +obj_type = BLE_STORE_OBJ_TYPE_OUR_SEC; +} else { +obj_type = BLE_STORE_OBJ_TYPE_PEER_SEC; +} + +rc = ble_store_util_count(obj_type, ); +if (rc != 0) { +return rc; +} + +/* Pessimistically assume all active procs will persist bonds. */ +ble_hs_lock(); +count += ble_sm_num_procs(); Review comment: count is not initialized This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow
rymanluk commented on a change in pull request #279: BLE Host - Policy for SM key overflow URL: https://github.com/apache/incubator-mynewt-core/pull/279#discussion_r119288035 ## File path: net/nimble/host/src/ble_store.c ## @@ -28,49 +28,98 @@ ble_store_read(int obj_type, const union ble_store_key *key, { int rc; +ble_hs_lock(); + if (ble_hs_cfg.store_read_cb == NULL) { rc = BLE_HS_ENOTSUP; } else { rc = ble_hs_cfg.store_read_cb(obj_type, key, val); } +ble_hs_unlock(); + return rc; } int ble_store_write(int obj_type, const union ble_store_value *val) { +struct ble_store_status_event event; int rc; if (ble_hs_cfg.store_write_cb == NULL) { -rc = BLE_HS_ENOTSUP; -} else { -rc = ble_hs_cfg.store_write_cb(obj_type, val); +return BLE_HS_ENOTSUP; } -return rc; +while (1) { +ble_hs_lock(); +rc = ble_hs_cfg.store_write_cb(obj_type, val); +ble_hs_unlock(); + +switch (rc) { +case 0: +return 0; +case BLE_HS_ESTORE_CAP: +/* Record didn't fit. Give the application the opportunity to free + * up some space. + */ +event.obj_type = obj_type; +event.event_code = BLE_STORE_EVENT_OVERFLOW; +event.value = val; +rc = ble_store_status(); +if (rc != 0) { +return rc; +} + +/* Application made room for the record; try again. */ +break; + +default: +return rc; +} +} } int ble_store_delete(int obj_type, const union ble_store_key *key) { int rc; +ble_hs_lock(); Review comment: same as above This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services