[GitHub] ccollins476ad commented on a change in pull request #279: BLE Host - Policy for SM key overflow
ccollins476ad 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_r122791440
##
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:
OK, I understand. Thanks, that is a good idea. I have added two functions:
```
int ble_store_overflow_event(int obj_type, const union ble_store_value
*value);
int ble_store_overflow_next_event(int obj_type, uint16_t conn_handle);
```
and made `ble_store_status` static.
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] ccollins476ad commented on a change in pull request #279: BLE Host - Policy for SM key overflow
ccollins476ad 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_r122786922
##
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:
I really do want to make changes that you suggest, but, subjectively, I'm
just not sure its an improvement in the signal-to-noise of the code. In my
mind, the concept of "validity" doesn't really apply here. This is a
discriminated union, and the user needs to know which field can be validly
read. That selection is made via the "event_code" field (i.e., that is the
descriminator). The "obj_type" field has no effect on the validity of each
member of the union.
Also, I'm not sure we'll never use this event for the CCCD object type. I
think we should use it for all object types where possible. The current CCCD
persistence code doesn't do a good job of handling all the error cases, so
hopefully we can improve it going forward.
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] ccollins476ad commented on a change in pull request #279: BLE Host - Policy for SM key overflow
ccollins476ad 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_r122237714
##
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:
I was asking which of those two you would like to 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] ccollins476ad commented on a change in pull request #279: BLE Host - Policy for SM key overflow
ccollins476ad 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_r121187064
##
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:
I didn't think that information is necessary for the reader to understand
the code. The obj_type field isn't part of the union, so is valid for all
events. The union fields, on the other hand, are not always valid; the user
needs to know how to determine which one to use.
I am a bit confused about what you are saying here. Which of these are you
saying I should do:
1. Valid for all possible obj_types
2. Valid for the following obj_types:
* BLE_STORE_OBJ_TYPE_OUR_SEC
* BLE_STORE_OBJ_TYPE_PEER_SEC
* BLE_STORE_OBJ_TYPE_CCCD
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] ccollins476ad commented on a change in pull request #279: BLE Host - Policy for SM key overflow
ccollins476ad 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_r121185868
##
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:
Isn't that what ble_store_status() does? Or were you thinking this function
would take parameters and fill the event object in itself?
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] ccollins476ad commented on a change in pull request #279: BLE Host - Policy for SM key overflow
ccollins476ad 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_r120217422
##
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:
I don't think BLE_STORE_EVENT_OVERFLOW_NEXT is particularly unclear. It is
so named because the next store operation will overflow. The user may confuse
this event type with the other (BLE_STORE_EVENT_OVERFLOW) because the names are
similar, but I think this is going to be hard to prevent. After all, the
meanings of the events are similar!
If there is a specific name you like, I'm happy to use it. Personally, I am
OK with the current one, or several of the suggestions.
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] ccollins476ad commented on a change in pull request #279: BLE Host - Policy for SM key overflow
ccollins476ad 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_r119369837
##
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:
It is initialized by the prior call to `ble_store_util_count()`.
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] ccollins476ad commented on a change in pull request #279: BLE Host - Policy for SM key overflow
ccollins476ad 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_r119369396
##
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:
I wasn't sure if this event would be used for other record types as well
(not just bonds). Since the first one (OVERFLOW) is used for multiple record
types, I opted to keep both generic.
I do agree BONDING is more clear. However, my only concern is that we'll
have to rename it again (change the API) if we want to use it for other record
types in the future.
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
