[general] Fix calls to zjs_signal_callback #903
Conversation
I introduced two bugs to zjs_dgram.c because of my recent argument validation patch, where I got confused about whether zjs_signal_callback was taking its size in bytes (right!) vs. number of values (wrong!). We use bytes because the callback mechanism doesn't only hold JS values but sometimes C values that might be different sizes than 4 bytes. I found at least one other bug, and then also cleaned up the size calculations to not rely on magic numbers when a fixed array (etc.) was available for the compiler to do the math, per pfalcon's suggestion. Signed-off-by: Geoff Gustafson <geoff@linux.intel.com>
| @@ -142,8 +142,7 @@ static void ipm_msg_receive_callback(void *context, uint32_t id, | |||
| case TYPE_AIO_PIN_EVENT_VALUE_CHANGE: | |||
| handle->value = (double)pin_value; | |||
| ZVAL num = jerry_create_number(handle->value); | |||
| zjs_signal_callback(handle->callback_id, &num, | |||
| sizeof(jerry_value_t)); | |||
| zjs_signal_callback(handle->callback_id, &num, sizeof(num)); | |||
There was a problem hiding this comment.
So this kind of change is just removing some magic; rely on the compiler's knowledge of the type of num, a little cleaner.
| @@ -492,7 +492,7 @@ static void zjs_ble_connected(struct bt_conn *conn, uint8_t err) | |||
| char addr[BT_ADDR_LE_STR_LEN]; | |||
| bt_addr_le_to_str(bt_conn_get_dst(conn), addr, sizeof(addr)); | |||
| ble_conn.default_conn = bt_conn_ref(conn); | |||
| zjs_signal_callback(ble_conn.connected_cb_id, &addr, sizeof(addr)); | |||
| zjs_signal_callback(ble_conn.connected_cb_id, addr, sizeof(addr)); | |||
There was a problem hiding this comment.
In C addr and &addr are exactly the same thing for a fixed array like this. I think addr is cleaner because then the syntax the same as for a dynamic array, char *addr;
There was a problem hiding this comment.
They aren't even the same, addr on its own casts to "pointer to type of element", while type of &addr is "pointer to array of elements, of given size". Difference is negligible for C, but C++ would start to complain here...
| @@ -134,7 +134,7 @@ static void udp_received(struct net_context *context, | |||
| net_nbuf_unref(net_buf); | |||
|
|
|||
| jerry_value_t args[2] = {buf_js, rinfo}; | |||
| zjs_signal_callback(handle->message_cb_id, args, 2); | |||
| zjs_signal_callback(handle->message_cb_id, args, sizeof(args)); | |||
There was a problem hiding this comment.
For arrays, like this it's much better to use sizeof than the magic number two. And in this case I'm fixing the bug I introduced recently with #842.
| @@ -227,7 +227,7 @@ static void udp_sent(struct net_context *context, int status, void *token, | |||
|
|
|||
| zjs_callback_id id = zjs_add_callback_once((jerry_value_t)user_data, | |||
| ZJS_UNDEFINED, NULL, NULL); | |||
| zjs_signal_callback(id, &rval, 1); | |||
| zjs_signal_callback(id, &rval, sizeof(rval)); | |||
There was a problem hiding this comment.
I had earlier created this bug in #844.
| @@ -268,7 +268,7 @@ static void zjs_sensor_trigger_error(jerry_value_t obj, | |||
| zjs_set_property(error_obj, "message", message_val); | |||
| zjs_set_property(event, "error", error_obj); | |||
| zjs_callback_id id = zjs_add_callback_once(func, obj, NULL, NULL); | |||
| zjs_signal_callback(id, &event, 1); | |||
| zjs_signal_callback(id, &event, sizeof(event)); | |||
There was a problem hiding this comment.
This was yet another bug of the same type - thinking signal_callback takes argc when it really takes size in bytes.
|
+1 |
|
In case I don't get back to this right away, I've verified analog input with AIO.js and BLE w/ WebBluetoothGroveLcdDemo.js so far. |
|
Verified dgram as well, I think, by testing with qemu. But I first had to fix #911 to get it to work. @jimmy-huang if you're satifisied with the sensor portion this should be ready. |
|
+1 |
I introduced two bugs to zjs_dgram.c because of my recent argument
validation patch, where I got confused about whether zjs_signal_callback
was taking its size in bytes (right!) vs. number of values (wrong!).
We use bytes because the callback mechanism doesn't only hold JS values
but sometimes C values that might be different sizes than 4 bytes.
I found at least one other bug, and then also cleaned up the size
calculations to not rely on magic numbers when a fixed array (etc.) was
available for the compiler to do the math, per pfalcon's suggestion.
Signed-off-by: Geoff Gustafson geoff@linux.intel.com