From 945898f8fd99d6357b834a546c1a80a9f01d06b0 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Mon, 9 Apr 2018 21:18:13 +0200 Subject: [PATCH 1/4] Always initialize the output parameters I/O functions with output parameters, should always initialize those output parameters, even when an error is returned. This prevents the (accidental) use of uninitialized variables, whenever the caller forgets to check the return code. As a nice side effect, the use of a local variable guarantees that the underlying I/O implementation will always receive a valid pointer. --- src/iostream.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/src/iostream.c b/src/iostream.c index 9681d02..954a85f 100644 --- a/src/iostream.c +++ b/src/iostream.c @@ -131,19 +131,41 @@ dc_iostream_set_rts (dc_iostream_t *iostream, unsigned int value) dc_status_t dc_iostream_get_lines (dc_iostream_t *iostream, unsigned int *value) { - if (iostream == NULL || iostream->vtable->get_lines == NULL) - return DC_STATUS_UNSUPPORTED; + dc_status_t status = DC_STATUS_SUCCESS; + unsigned int lines = 0; - return iostream->vtable->get_lines (iostream, value); + if (iostream == NULL || iostream->vtable->get_lines == NULL) { + status = DC_STATUS_UNSUPPORTED; + goto out; + } + + status = iostream->vtable->get_lines (iostream, &lines); + +out: + if (value) + *value = lines; + + return status; } dc_status_t dc_iostream_get_available (dc_iostream_t *iostream, size_t *value) { - if (iostream == NULL || iostream->vtable->get_available == NULL) - return DC_STATUS_UNSUPPORTED; + dc_status_t status = DC_STATUS_SUCCESS; + size_t available = 0; - return iostream->vtable->get_available (iostream, value); + if (iostream == NULL || iostream->vtable->get_available == NULL) { + status = DC_STATUS_UNSUPPORTED; + goto out; + } + + status = iostream->vtable->get_available (iostream, &available); + +out: + if (value) + *value = available; + + return status; } dc_status_t From 1908394af439a0f34d61e20015ee8d0a70a73bb8 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Mon, 9 Apr 2018 21:25:41 +0200 Subject: [PATCH 2/4] Add some extra logging --- src/iostream.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/iostream.c b/src/iostream.c index 954a85f..f441916 100644 --- a/src/iostream.c +++ b/src/iostream.c @@ -25,6 +25,7 @@ #include "iostream-private.h" #include "context-private.h" +#include "platform.h" dc_iostream_t * dc_iostream_allocate (dc_context_t *context, const dc_iostream_vtable_t *vtable, dc_transport_t transport) @@ -141,6 +142,8 @@ dc_iostream_get_lines (dc_iostream_t *iostream, unsigned int *value) status = iostream->vtable->get_lines (iostream, &lines); + INFO (iostream->context, "Lines: value=%u", lines); + out: if (value) *value = lines; @@ -161,6 +164,8 @@ dc_iostream_get_available (dc_iostream_t *iostream, size_t *value) status = iostream->vtable->get_available (iostream, &available); + INFO (iostream->context, "Available: value=" DC_PRINTF_SIZE, available); + out: if (value) *value = available; From 56d194d377a4ee4c01538990612ddf3ccf201032 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Mon, 9 Apr 2018 22:02:19 +0200 Subject: [PATCH 3/4] Use a NULL pointer for the no-op implementation For most I/O stream implementations the serial communication specific functions are meaningless. Implementing them as no-ops allows the dive computer backends the call the I/O stream functions unconditionally. However, implementing the no-op with a dummy function returning DC_STATUS_SUCCESS, does not only add some (small) overhead at runtime, but also requires many such functions. This is inconvenient and the same result can easily be obtained by using a NULL pointer instead. The consequence is that the logic is reversed now. To obtain the previous behaviour of returning the DC_STATUS_UNSUPPORTED error code again, you'll need to implement a dummy function. But that's fine because it's the less common case. --- src/bluetooth.c | 18 ++++++++-------- src/custom.c | 34 ++++++++++++++--------------- src/iostream.c | 22 ++++++++----------- src/irda.c | 18 ++++++++-------- src/socket.c | 57 ------------------------------------------------- src/socket.h | 27 ----------------------- 6 files changed, 43 insertions(+), 133 deletions(-) diff --git a/src/bluetooth.c b/src/bluetooth.c index b55bf86..99ee81e 100644 --- a/src/bluetooth.c +++ b/src/bluetooth.c @@ -99,18 +99,18 @@ static const dc_iterator_vtable_t dc_bluetooth_iterator_vtable = { static const dc_iostream_vtable_t dc_bluetooth_vtable = { sizeof(dc_socket_t), dc_socket_set_timeout, /* set_timeout */ - dc_socket_set_latency, /* set_latency */ - dc_socket_set_break, /* set_break */ - dc_socket_set_dtr, /* set_dtr */ - dc_socket_set_rts, /* set_rts */ - dc_socket_get_lines, /* get_lines */ + NULL, /* set_latency */ + NULL, /* set_break */ + NULL, /* set_dtr */ + NULL, /* set_rts */ + NULL, /* get_lines */ dc_socket_get_available, /* get_received */ - dc_socket_configure, /* configure */ + NULL, /* configure */ dc_socket_read, /* read */ dc_socket_write, /* write */ - dc_socket_flush, /* flush */ - dc_socket_purge, /* purge */ - dc_socket_sleep, /* sleep */ + NULL, /* flush */ + NULL, /* purge */ + NULL, /* sleep */ dc_socket_close, /* close */ }; diff --git a/src/custom.c b/src/custom.c index 1e03aa7..1931d84 100644 --- a/src/custom.c +++ b/src/custom.c @@ -99,7 +99,7 @@ dc_custom_set_timeout (dc_iostream_t *abstract, int timeout) dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.set_timeout == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.set_timeout (custom->userdata, timeout); } @@ -110,7 +110,7 @@ dc_custom_set_latency (dc_iostream_t *abstract, unsigned int value) dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.set_latency == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.set_latency (custom->userdata, value); } @@ -121,7 +121,7 @@ dc_custom_set_break (dc_iostream_t *abstract, unsigned int value) dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.set_break == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.set_break (custom->userdata, value); } @@ -132,7 +132,7 @@ dc_custom_set_dtr (dc_iostream_t *abstract, unsigned int value) dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.set_dtr == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.set_dtr (custom->userdata, value); } @@ -143,7 +143,7 @@ dc_custom_set_rts (dc_iostream_t *abstract, unsigned int value) dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.set_rts == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.set_rts (custom->userdata, value); } @@ -154,7 +154,7 @@ dc_custom_get_lines (dc_iostream_t *abstract, unsigned int *value) dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.get_lines == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.get_lines (custom->userdata, value); } @@ -165,7 +165,7 @@ dc_custom_get_available (dc_iostream_t *abstract, size_t *value) dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.get_available == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.get_available (custom->userdata, value); } @@ -176,7 +176,7 @@ dc_custom_configure (dc_iostream_t *abstract, unsigned int baudrate, unsigned in dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.configure == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.configure (custom->userdata, baudrate, databits, parity, stopbits, flowcontrol); } @@ -187,7 +187,7 @@ dc_custom_read (dc_iostream_t *abstract, void *data, size_t size, size_t *actual dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.read == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.read (custom->userdata, data, size, actual); } @@ -198,7 +198,7 @@ dc_custom_write (dc_iostream_t *abstract, const void *data, size_t size, size_t dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.write == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.write (custom->userdata, data, size, actual); } @@ -209,7 +209,7 @@ dc_custom_flush (dc_iostream_t *abstract) dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.flush == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.flush (custom->userdata); } @@ -220,7 +220,7 @@ dc_custom_purge (dc_iostream_t *abstract, dc_direction_t direction) dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.purge == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.purge (custom->userdata, direction); } @@ -231,7 +231,7 @@ dc_custom_sleep (dc_iostream_t *abstract, unsigned int milliseconds) dc_custom_t *custom = (dc_custom_t *) abstract; if (custom->callbacks.sleep == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; return custom->callbacks.sleep (custom->userdata, milliseconds); } @@ -239,12 +239,10 @@ dc_custom_sleep (dc_iostream_t *abstract, unsigned int milliseconds) static dc_status_t dc_custom_close (dc_iostream_t *abstract) { - dc_status_t status = DC_STATUS_SUCCESS; dc_custom_t *custom = (dc_custom_t *) abstract; - if (custom->callbacks.close) { - status = custom->callbacks.close (custom->userdata); - } + if (custom->callbacks.close == NULL) + return DC_STATUS_SUCCESS; - return status; + return custom->callbacks.close (custom->userdata); } diff --git a/src/iostream.c b/src/iostream.c index f441916..e0ba7fc 100644 --- a/src/iostream.c +++ b/src/iostream.c @@ -78,7 +78,7 @@ dc_status_t dc_iostream_set_timeout (dc_iostream_t *iostream, int timeout) { if (iostream == NULL || iostream->vtable->set_timeout == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; INFO (iostream->context, "Timeout: value=%i", timeout); @@ -89,7 +89,7 @@ dc_status_t dc_iostream_set_latency (dc_iostream_t *iostream, unsigned int value) { if (iostream == NULL || iostream->vtable->set_latency == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; INFO (iostream->context, "Latency: value=%i", value); @@ -100,7 +100,7 @@ dc_status_t dc_iostream_set_break (dc_iostream_t *iostream, unsigned int value) { if (iostream == NULL || iostream->vtable->set_break == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; INFO (iostream->context, "Break: value=%i", value); @@ -111,7 +111,7 @@ dc_status_t dc_iostream_set_dtr (dc_iostream_t *iostream, unsigned int value) { if (iostream == NULL || iostream->vtable->set_dtr == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; INFO (iostream->context, "DTR: value=%i", value); @@ -122,7 +122,7 @@ dc_status_t dc_iostream_set_rts (dc_iostream_t *iostream, unsigned int value) { if (iostream == NULL || iostream->vtable->set_rts == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; INFO (iostream->context, "RTS: value=%i", value); @@ -136,7 +136,6 @@ dc_iostream_get_lines (dc_iostream_t *iostream, unsigned int *value) unsigned int lines = 0; if (iostream == NULL || iostream->vtable->get_lines == NULL) { - status = DC_STATUS_UNSUPPORTED; goto out; } @@ -158,7 +157,6 @@ dc_iostream_get_available (dc_iostream_t *iostream, size_t *value) size_t available = 0; if (iostream == NULL || iostream->vtable->get_available == NULL) { - status = DC_STATUS_UNSUPPORTED; goto out; } @@ -177,7 +175,7 @@ dc_status_t dc_iostream_configure (dc_iostream_t *iostream, unsigned int baudrate, unsigned int databits, dc_parity_t parity, dc_stopbits_t stopbits, dc_flowcontrol_t flowcontrol) { if (iostream == NULL || iostream->vtable->configure == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; INFO (iostream->context, "Configure: baudrate=%i, databits=%i, parity=%i, stopbits=%i, flowcontrol=%i", baudrate, databits, parity, stopbits, flowcontrol); @@ -192,7 +190,6 @@ dc_iostream_read (dc_iostream_t *iostream, void *data, size_t size, size_t *actu size_t nbytes = 0; if (iostream == NULL || iostream->vtable->read == NULL) { - status = DC_STATUS_UNSUPPORTED; goto out; } @@ -214,7 +211,6 @@ dc_iostream_write (dc_iostream_t *iostream, const void *data, size_t size, size_ size_t nbytes = 0; if (iostream == NULL || iostream->vtable->read == NULL) { - status = DC_STATUS_UNSUPPORTED; goto out; } @@ -233,7 +229,7 @@ dc_status_t dc_iostream_flush (dc_iostream_t *iostream) { if (iostream == NULL || iostream->vtable->flush == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; INFO (iostream->context, "Flush: none"); @@ -244,7 +240,7 @@ dc_status_t dc_iostream_purge (dc_iostream_t *iostream, dc_direction_t direction) { if (iostream == NULL || iostream->vtable->purge == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; INFO (iostream->context, "Purge: direction=%u", direction); @@ -255,7 +251,7 @@ dc_status_t dc_iostream_sleep (dc_iostream_t *iostream, unsigned int milliseconds) { if (iostream == NULL || iostream->vtable->sleep == NULL) - return DC_STATUS_UNSUPPORTED; + return DC_STATUS_SUCCESS; INFO (iostream->context, "Sleep: value=%u", milliseconds); diff --git a/src/irda.c b/src/irda.c index a5cf803..1f19cc3 100644 --- a/src/irda.c +++ b/src/irda.c @@ -91,18 +91,18 @@ static const dc_iterator_vtable_t dc_irda_iterator_vtable = { static const dc_iostream_vtable_t dc_irda_vtable = { sizeof(dc_socket_t), dc_socket_set_timeout, /* set_timeout */ - dc_socket_set_latency, /* set_latency */ - dc_socket_set_break, /* set_break */ - dc_socket_set_dtr, /* set_dtr */ - dc_socket_set_rts, /* set_rts */ - dc_socket_get_lines, /* get_lines */ + NULL, /* set_latency */ + NULL, /* set_break */ + NULL, /* set_dtr */ + NULL, /* set_rts */ + NULL, /* get_lines */ dc_socket_get_available, /* get_received */ - dc_socket_configure, /* configure */ + NULL, /* configure */ dc_socket_read, /* read */ dc_socket_write, /* write */ - dc_socket_flush, /* flush */ - dc_socket_purge, /* purge */ - dc_socket_sleep, /* sleep */ + NULL, /* flush */ + NULL, /* purge */ + NULL, /* sleep */ dc_socket_close, /* close */ }; #endif diff --git a/src/socket.c b/src/socket.c index 1842a75..948aa46 100644 --- a/src/socket.c +++ b/src/socket.c @@ -163,39 +163,6 @@ dc_socket_set_timeout (dc_iostream_t *abstract, int timeout) return DC_STATUS_SUCCESS; } -dc_status_t -dc_socket_set_latency (dc_iostream_t *iostream, unsigned int value) -{ - return DC_STATUS_SUCCESS; -} - -dc_status_t -dc_socket_set_break (dc_iostream_t *iostream, unsigned int value) -{ - return DC_STATUS_SUCCESS; -} - -dc_status_t -dc_socket_set_dtr (dc_iostream_t *iostream, unsigned int value) -{ - return DC_STATUS_SUCCESS; -} - -dc_status_t -dc_socket_set_rts (dc_iostream_t *iostream, unsigned int value) -{ - return DC_STATUS_SUCCESS; -} - -dc_status_t -dc_socket_get_lines (dc_iostream_t *iostream, unsigned int *value) -{ - if (value) - *value = 0; - - return DC_STATUS_SUCCESS; -} - dc_status_t dc_socket_get_available (dc_iostream_t *abstract, size_t *value) { @@ -219,12 +186,6 @@ dc_socket_get_available (dc_iostream_t *abstract, size_t *value) return DC_STATUS_SUCCESS; } -dc_status_t -dc_socket_configure (dc_iostream_t *abstract, unsigned int baudrate, unsigned int databits, dc_parity_t parity, dc_stopbits_t stopbits, dc_flowcontrol_t flowcontrol) -{ - return DC_STATUS_SUCCESS; -} - dc_status_t dc_socket_read (dc_iostream_t *abstract, void *data, size_t size, size_t *actual) { @@ -332,21 +293,3 @@ out: return status; } - -dc_status_t -dc_socket_flush (dc_iostream_t *abstract) -{ - return DC_STATUS_SUCCESS; -} - -dc_status_t -dc_socket_purge (dc_iostream_t *abstract, dc_direction_t direction) -{ - return DC_STATUS_SUCCESS; -} - -dc_status_t -dc_socket_sleep (dc_iostream_t *abstract, unsigned int timeout) -{ - return DC_STATUS_SUCCESS; -} diff --git a/src/socket.h b/src/socket.h index 626003c..5f5421d 100644 --- a/src/socket.h +++ b/src/socket.h @@ -104,42 +104,15 @@ dc_socket_connect (dc_iostream_t *iostream, const struct sockaddr *addr, s_sockl dc_status_t dc_socket_set_timeout (dc_iostream_t *iostream, int timeout); -dc_status_t -dc_socket_set_latency (dc_iostream_t *iostream, unsigned int value); - -dc_status_t -dc_socket_set_break (dc_iostream_t *iostream, unsigned int value); - -dc_status_t -dc_socket_set_dtr (dc_iostream_t *iostream, unsigned int value); - -dc_status_t -dc_socket_set_rts (dc_iostream_t *iostream, unsigned int value); - -dc_status_t -dc_socket_get_lines (dc_iostream_t *iostream, unsigned int *value); - dc_status_t dc_socket_get_available (dc_iostream_t *iostream, size_t *value); -dc_status_t -dc_socket_configure (dc_iostream_t *iostream, unsigned int baudrate, unsigned int databits, dc_parity_t parity, dc_stopbits_t stopbits, dc_flowcontrol_t flowcontrol); - dc_status_t dc_socket_read (dc_iostream_t *iostream, void *data, size_t size, size_t *actual); dc_status_t dc_socket_write (dc_iostream_t *iostream, const void *data, size_t size, size_t *actual); -dc_status_t -dc_socket_flush (dc_iostream_t *iostream); - -dc_status_t -dc_socket_purge (dc_iostream_t *iostream, dc_direction_t direction); - -dc_status_t -dc_socket_sleep (dc_iostream_t *iostream, unsigned int milliseconds); - dc_status_t dc_socket_close (dc_iostream_t *iostream); From 29f781f8038ad010167cf3fd486f2bd7e8fba4b3 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Mon, 9 Apr 2018 22:48:16 +0200 Subject: [PATCH 4/4] Fix a typo in the comments --- src/bluetooth.c | 2 +- src/custom.c | 2 +- src/irda.c | 2 +- src/serial_posix.c | 2 +- src/serial_win32.c | 2 +- src/usbhid.c | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/bluetooth.c b/src/bluetooth.c index 99ee81e..088d027 100644 --- a/src/bluetooth.c +++ b/src/bluetooth.c @@ -104,7 +104,7 @@ static const dc_iostream_vtable_t dc_bluetooth_vtable = { NULL, /* set_dtr */ NULL, /* set_rts */ NULL, /* get_lines */ - dc_socket_get_available, /* get_received */ + dc_socket_get_available, /* get_available */ NULL, /* configure */ dc_socket_read, /* read */ dc_socket_write, /* write */ diff --git a/src/custom.c b/src/custom.c index 1931d84..8db276b 100644 --- a/src/custom.c +++ b/src/custom.c @@ -58,7 +58,7 @@ static const dc_iostream_vtable_t dc_custom_vtable = { dc_custom_set_dtr, /* set_dtr */ dc_custom_set_rts, /* set_rts */ dc_custom_get_lines, /* get_lines */ - dc_custom_get_available, /* get_received */ + dc_custom_get_available, /* get_available */ dc_custom_configure, /* configure */ dc_custom_read, /* read */ dc_custom_write, /* write */ diff --git a/src/irda.c b/src/irda.c index 1f19cc3..eedb703 100644 --- a/src/irda.c +++ b/src/irda.c @@ -96,7 +96,7 @@ static const dc_iostream_vtable_t dc_irda_vtable = { NULL, /* set_dtr */ NULL, /* set_rts */ NULL, /* get_lines */ - dc_socket_get_available, /* get_received */ + dc_socket_get_available, /* get_available */ NULL, /* configure */ dc_socket_read, /* read */ dc_socket_write, /* write */ diff --git a/src/serial_posix.c b/src/serial_posix.c index ff954ef..46ec3de 100644 --- a/src/serial_posix.c +++ b/src/serial_posix.c @@ -121,7 +121,7 @@ static const dc_iostream_vtable_t dc_serial_vtable = { dc_serial_set_dtr, /* set_dtr */ dc_serial_set_rts, /* set_rts */ dc_serial_get_lines, /* get_lines */ - dc_serial_get_available, /* get_received */ + dc_serial_get_available, /* get_available */ dc_serial_configure, /* configure */ dc_serial_read, /* read */ dc_serial_write, /* write */ diff --git a/src/serial_win32.c b/src/serial_win32.c index ff52b41..124857a 100644 --- a/src/serial_win32.c +++ b/src/serial_win32.c @@ -91,7 +91,7 @@ static const dc_iostream_vtable_t dc_serial_vtable = { dc_serial_set_dtr, /* set_dtr */ dc_serial_set_rts, /* set_rts */ dc_serial_get_lines, /* get_lines */ - dc_serial_get_available, /* get_received */ + dc_serial_get_available, /* get_available */ dc_serial_configure, /* configure */ dc_serial_read, /* read */ dc_serial_write, /* write */ diff --git a/src/usbhid.c b/src/usbhid.c index 0338bfe..a3ddc74 100644 --- a/src/usbhid.c +++ b/src/usbhid.c @@ -142,7 +142,7 @@ static const dc_iostream_vtable_t dc_usbhid_vtable = { NULL, /* set_dtr */ NULL, /* set_rts */ NULL, /* get_lines */ - NULL, /* get_received */ + NULL, /* get_available */ NULL, /* configure */ dc_usbhid_read, /* read */ dc_usbhid_write, /* write */