[PATCH] libcurl channel: enable connection timeout option

[Sava Jakovljev]

* Add new configuration option for Suricatta which enables users to set
connection timeout in seconds.
* Update documentation.
---
corelib/channel_curl.c | 9 +++++++++
doc/source/swupdate.rst | 11 +++++++++--
examples/configuration/swupdate.cfg | 8 ++++++--
include/channel_curl.h | 3 +++
suricatta/server_hawkbit.c | 19 +++++++++++++++----
5 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c
index d2c1ab0..ff5f6d1 100644
--- a/corelib/channel_curl.c
+++ b/corelib/channel_curl.c
@@ -534,6 +534,13 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da
"this is most probably not what you want. "
"Adapted it to %us instead.\n", SPEED_LOW_TIME_SEC);
}
+ if (channel_data->connection_timeout <= 0) {
+ channel_data->connection_timeout = DEFAULT_CONNECTION_TIMEOUT;
+ DEBUG("cURL's connection timeout is disabled or invalid."
+ "This is most probably not what you want. "
+ "Adapted it to %us instead.\n", DEFAULT_CONNECTION_TIMEOUT);
+ }
+
channel_curl_t *channel_curl = this->priv;
channel_op_res_t result = CHANNEL_OK;
if ((curl_easy_setopt(channel_curl->handle, CURLOPT_URL,
@@ -544,6 +551,8 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da
SPEED_LOW_BYTES_SEC) != CURLE_OK) ||
(curl_easy_setopt(channel_curl->handle, CURLOPT_LOW_SPEED_TIME,
channel_data->low_speed_timeout) != CURLE_OK) ||
+ (curl_easy_setopt(channel_curl->handle, CURLOPT_CONNECTTIMEOUT,
+ channel_data->connection_timeout) != CURLE_OK) ||
(curl_easy_setopt(channel_curl->handle, CURLOPT_HTTPHEADER,
channel_curl->header) != CURLE_OK) ||
(curl_easy_setopt(channel_curl->handle, CURLOPT_MAXREDIRS, -1) !=
diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
index d20e250..0b8e0c1 100644
--- a/doc/source/swupdate.rst
+++ b/doc/source/swupdate.rst
@@ -184,7 +184,7 @@ Images fully streamed
---------------------

In case of remote update, SWUpdate extracts relevant images from the stream
-and copies them into the directory pointed to by the environment variable
+and copies them into the directory pointed to by the environment variable
``TMPDIR`` (if unset, to ``/tmp``) before calling the handlers.
This guarantee that an update is initiated only if all parts are present and
correct. However, on some systems with less resources, the amount of RAM
@@ -643,6 +643,13 @@ Mandatory arguments are marked with '\*':
| | | Suricatta is started with initial state of |
| | | STATE_WAIT ("-c 6"), this value is ignored.|
+-------------------------+----------+--------------------------------------------+
+| -s <seconds> | integer | Connection timeout to use in seconds. |
+| | | Default value is 15 seconds. |
+| | | NOTE: it is not possible for Suricatta to |
+| | | respond to external program API requests |
+| | | during this period - adapt this value to |
+| | | your use case! |
++-------------------------+----------+--------------------------------------------+


systemd Integration
@@ -697,7 +704,7 @@ files are also handed over on a "regular" start of SWUpdate via
``systemctl start swupdate.service``.

Note that the socket paths in the two ``ListenStream=`` directives
-have to match the socket paths ``CONFIG_SOCKET_CTRL_PATH`` and
+have to match the socket paths ``CONFIG_SOCKET_CTRL_PATH`` and
``CONFIG_SOCKET_PROGRESS_PATH`` in SWUpdate's configuration.
Here, the default socket path configuration is depicted.

diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg
index c0547c5..49f9b00 100644
--- a/examples/configuration/swupdate.cfg
+++ b/examples/configuration/swupdate.cfg
@@ -15,7 +15,7 @@
# enable sending logs to syslog daemon
# public-key-file : string
# file with public key for
-# image verification
+# image verification
# mtd-blacklist : list integers
# MTD devices where SWUpdate
# must not try to check for UBI filesystem.
@@ -120,7 +120,7 @@ identify : (
{ name = "swCompatibility"; value = "unknown";}
);

-#
+#
# suricatta section: setup for backend
#
# Currently, they refer to the Hawkbit agent.
@@ -169,6 +169,9 @@ identify : (
# initial-report-resend-period : integer
# Specify period between re-tryint to send initial state, specified with "-c" option,
# when connection to Hawkbit is not available. Default value is 10 seconds.
+# connection-timeout : integer
+# Specify server connection timeout. If no connection has been established in this
+# period, libcurl will consider connection unsuccessful. Default value is 15 seconds.

suricatta :
{
@@ -184,6 +187,7 @@ suricatta :
groupid = 1000;
enable = true;
initial-report-resend-period = 30;
+ connection-timeout = 10;
/*
cafile = "/etc/ssl/cafile";
sslkey = "/etc/ssl/sslkey";
diff --git a/include/channel_curl.h b/include/channel_curl.h
index 67cc971..c5ff881 100644
--- a/include/channel_curl.h
+++ b/include/channel_curl.h
@@ -14,6 +14,8 @@
#include <stdbool.h>
#include "swupdate_status.h"

+#define DEFAULT_CONNECTION_TIMEOUT 15
+
/* Curl Channel Implementation Private Header File.
*
* This is a "private" header for testability, i.e., the declarations and
@@ -60,6 +62,7 @@ typedef struct {
unsigned int method;
unsigned int retries;
unsigned int low_speed_timeout;
+ unsigned int connection_timeout;
channel_body_t format;
bool debug;
bool usessl;
diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c
index f0d10c0..1ae1a07 100644
--- a/suricatta/server_hawkbit.c
+++ b/suricatta/server_hawkbit.c
@@ -51,6 +51,7 @@ static struct option long_options[] = {
{"disable-token-for-dwl", no_argument, NULL, '1'},
{"cache", required_argument, NULL, '2'},
{"initial-report-resend-period", required_argument, NULL, 'm'},
+ {"connection-timeout", required_argument, NULL, 's'},
{NULL, 0, NULL, 0}};

static unsigned short mandatory_argument_count = 0;
@@ -130,7 +131,8 @@ static channel_data_t channel_data_defaults = {.debug = false,
.format = CHANNEL_PARSE_JSON,
.nocheckanswer = false,
.nofollow = false,
- .strictssl = true
+ .strictssl = true,
+ .connection_timeout = 0
};

static struct timeval server_time;
@@ -1599,10 +1601,12 @@ void server_print_help(void)
"\t --disable-token-for-dwl Do not send authentication header when downlloading SWU.\n"
"\t --cache <file> Use cache file as starting SWU\n"
"\t -m, --initial-report-resend-period <seconds> Time to wait prior to retry "
- "sending initial state with '-c' option (default: %ds).\n",
+ "sending initial state with '-c' option (default: %ds).\n"
+ "\t -s, --connection-timeout Set the server connection timeout (default: %ds).\n",
CHANNEL_DEFAULT_POLLING_INTERVAL, CHANNEL_DEFAULT_RESUME_TRIES,
CHANNEL_DEFAULT_RESUME_DELAY,
- INITIAL_STATUS_REPORT_WAIT_DELAY);
+ INITIAL_STATUS_REPORT_WAIT_DELAY,
+ DEFAULT_CONNECTION_TIMEOUT);
}

static int server_hawkbit_settings(void *elem, void __attribute__ ((__unused__)) *data)
@@ -1636,6 +1640,9 @@ static int server_hawkbit_settings(void *elem, void __attribute__ ((__unused__)
get_field(LIBCFG_PARSER, elem, "usetokentodwl",
&server_hawkbit.usetokentodwl);

+ get_field(LIBCFG_PARSER, elem, "connection-timeout",
+ &channel_data_defaults.connection_timeout);
+
GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "targettoken", tmp);
if (strlen(tmp))
SETSTRING(server_hawkbit.targettoken, tmp);
@@ -1683,7 +1690,7 @@ server_op_res_t server_start(char *fname, int argc, char *argv[])
/* reset to optind=1 to parse suricatta's argument vector */
optind = 1;
opterr = 0;
- while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:g:f:2:m:",
+ while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:g:f:2:m:s:",
long_options, NULL)) != -1) {
switch (choice) {
case 't':
@@ -1772,6 +1779,10 @@ server_op_res_t server_start(char *fname, int argc, char *argv[])
server_hawkbit.initial_report_resend_period =
(unsigned int)strtoul(optarg, NULL, 10);
break;
+ case 's':
+ channel_data_defaults.connection_timeout =
+ (unsigned int)strtoul(optarg, NULL, 10);
+ break;
/* Ignore not recognized options, they can be already parsed by the caller */
case '?':
break;
--
2.26.3

[Sava Jakovljev]

Hi Stefano,

What do you think about exposing other libcurl configuration options, like SPEED_LOW_TIME_SEC and others? I didn't want to do it in this patch, but maybe it would be good if we would make configuration of used curl channel configurable.

Best regards,

Sava Jakovljev

[Sava Jakovljev]

* Add new configuration option for Suricatta which enables users to set
connection timeout in seconds.
* Update documentation.

Signed-off-by: Sava Jakovljev <sava.ja...@teufel.de>

[Sava Jakovljev]

I submitted same patch as before, but this without missing Signed-Off.

Best regards,

Sava Jakovljev

[Stefano Babic]

Hi Sava,

I am just thinking about if the default value are changing the behavior
so far. At the moment, CURLOPT_CONNECTTIMEOUT is not used and according
to Curl documentation, this realizes to set a default built-in
connection time of 300 seconds. You have set it to 15 seconds, where
have you found this value ?

You can set channel_data->connection_timeout to zero if not set, and
curl should set itself the default value.

Correct, suricatta is from design a single-threaded application. This
has no drawbacks IMHO in the normal operations, that is polling the
server and performing the update (a cancel update request in case of
Hawkbit is polled during the update), but it is disturbing if more IPC
messages are flowing to suricatta from the application. If these cases
are becoming more frequently, I guess some rework will be required in
future - that at least suricatta answers with BUSY or it can answer
while the communication with server is still executed.

What is this 10 ?

Best regards,
Stefano Babic
--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================

[Sava Jakovljev]

Hi Stefano,

Yes, we set to 15 seconds, and that value is determined (in our case) somehow empirically. If you think some other value (other than the default one) is more reasonable, I am fine with that.

To explain a bit about the context, we are using Hawkbit in combination with STATE_WAIT, which requires an external program sending IPC to "activate" Suricatta before it starts polling the server.

During our testing, we experienced problems due to unresponsiveness of swupdate when our firewell would reject requests coming from swupdate - thus we introduced this timeout.

I agree that it would be better if Suricatta would answer with BUSY when it is unable to process the message - but as you said, that is a bit of rework and for now, IMHO this is an acceptable compromise, with additional benefit of providing users with more control options. Having other options exposed, like low speed timeout, etc., could also be beneficial for users, who could than tailor swupdate to use configuration they want. This would not be its biggest advantage or selling point, of course. What do you think? 

Regarding the 10 seconds in the example swupdate.cfg, it is just an illustration that you can set to some number of seconds, doesn't have any meaning. 

Best regards,

Sava Jakovljev

[Stefano Babic]

Hi Sava,

On 23.04.21 10:41, Sava Jakovljev wrote:
> Hi Stefano,
>
> Yes, we set to 15 seconds, and that value is determined (in our case)
> somehow empirically. If you think some other value (other than the
> default one) is more reasonable, I am fine with that.

I mean: SWUpdate should behave as in the past if the new parametr is not
set. I have frankly no idea and no way to test it in any conditions, but
I can imagine that a low level can break conditions where the devic eis
connected via low and unreliable network, like GSM, etc.

So my point is that the default should not be changed if the parameter
is not configured.

>
> To explain a bit about the context, we are using Hawkbit in combination
> with STATE_WAIT, which requires an external program sending IPC to
> "activate" Suricatta before it starts polling the server.

I supposed this, and yes, then there are limits to the single-threaded
implementation.

> During our testing, we experienced problems due to unresponsiveness of
> swupdate when our firewell would reject requests coming from swupdate -
> thus we introduced this timeout.
>

Ok

> I agree that it would be better if Suricatta would answer with BUSY when
> it is unable to process the message - but as you said, that is a bit of
> rework

Right, I agree.

> and for now, IMHO this is an acceptable compromise, with
> additional benefit of providing users with more control options.

Fine with me. It just be ensured that previous behavior is not
compromised and this becomes a new extended feature.

> Having
> other options exposed, like low speed timeout, etc., could also be
> beneficial for users, who could than tailor swupdate to use
> configuration they want. This would not be its biggest advantage or
> selling point, of course. What do you think?
>
> Regarding the 10 seconds in the example swupdate.cfg, it is just an
> illustration that you can set to some number of seconds, doesn't have
> any meaning.

Ah, ok, sorry, you're right.

Best regards,
Stefano Babic

> Phone: +49-8142-66989-53 <tel:+49%208142%206698953> Fax:
> +49-8142-66989-80 <tel:+49%208142%206698980> Email: sba...@denx.de
> =====================================================================
>
> --
> You received this message because you are subscribed to the Google
> Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to swupdate+u...@googlegroups.com
> <mailto:swupdate+u...@googlegroups.com>.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/swupdate/982fb416-4fc2-46d9-93e9-4dacd37ab7fan%40googlegroups.com
> <https://groups.google.com/d/msgid/swupdate/982fb416-4fc2-46d9-93e9-4dacd37ab7fan%40googlegroups.com?utm_medium=email&utm_source=footer>.

[Sava Jakovljev]

Hi Stefano,

Okay, I missed your point, and I agree - I will then remove setting CURLOPT_CONNECTTIMEOUT to swupdate-defined default value when user did not set the parameter explicitly.

Best regards,

Sava Jakovljev

[Sava Jakovljev]

* Add new configuration option for Suricatta which enables users to set
connection timeout in seconds.
* Update documentation.

Signed-off-by: Sava Jakovljev <sava.ja...@teufel.de>
---

corelib/channel_curl.c | 8 ++++++++
doc/source/swupdate.rst | 13 +++++++++++--
examples/configuration/swupdate.cfg | 9 +++++++--
include/channel_curl.h | 1 +
suricatta/server_hawkbit.c | 16 +++++++++++++---
5 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c
index d2c1ab0..e910f76 100644
--- a/corelib/channel_curl.c
+++ b/corelib/channel_curl.c
@@ -534,6 +534,7 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da

"this is most probably not what you want. "
"Adapted it to %us instead.\n", SPEED_LOW_TIME_SEC);
}
+

channel_curl_t *channel_curl = this->priv;
channel_op_res_t result = CHANNEL_OK;
if ((curl_easy_setopt(channel_curl->handle, CURLOPT_URL,

@@ -566,6 +567,13 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da
goto cleanup;
}

+ if (channel_data->connection_timeout > 0 &&
+ (curl_easy_setopt(channel_curl->handle, CURLOPT_CONNECTTIMEOUT,
+ channel_data->connection_timeout) != CURLE_OK)) {
+ result = CHANNEL_EINIT;
+ goto cleanup;
+ }
+
if (channel_data->debug) {
(void)curl_easy_setopt(channel_curl->handle, CURLOPT_VERBOSE, 1L);
}
diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
index d20e250..bed3e98 100644

--- a/doc/source/swupdate.rst
+++ b/doc/source/swupdate.rst
@@ -184,7 +184,7 @@ Images fully streamed
---------------------

In case of remote update, SWUpdate extracts relevant images from the stream
-and copies them into the directory pointed to by the environment variable
+and copies them into the directory pointed to by the environment variable
``TMPDIR`` (if unset, to ``/tmp``) before calling the handlers.
This guarantee that an update is initiated only if all parts are present and
correct. However, on some systems with less resources, the amount of RAM

@@ -643,6 +643,15 @@ Mandatory arguments are marked with '\*':

| | | Suricatta is started with initial state of |
| | | STATE_WAIT ("-c 6"), this value is ignored.|
+-------------------------+----------+--------------------------------------------+
+| -s <seconds> | integer | Connection timeout to use in seconds. |

+| | | If user doesn't set this option, default |
+| | | libcurl connection timeout value of 300 |
+| | | seconds is used. |

+| | | NOTE: it is not possible for Suricatta to |
+| | | respond to external program API requests |
+| | | during this period - adapt this value to |
+| | | your use case! |
++-------------------------+----------+--------------------------------------------+

systemd Integration
@@ -697,7 +706,7 @@ files are also handed over on a "regular" start of SWUpdate via

``systemctl start swupdate.service``.

Note that the socket paths in the two ``ListenStream=`` directives
-have to match the socket paths ``CONFIG_SOCKET_CTRL_PATH`` and
+have to match the socket paths ``CONFIG_SOCKET_CTRL_PATH`` and
``CONFIG_SOCKET_PROGRESS_PATH`` in SWUpdate's configuration.
Here, the default socket path configuration is depicted.

diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg

index c0547c5..2e12e15 100644

--- a/examples/configuration/swupdate.cfg
+++ b/examples/configuration/swupdate.cfg
@@ -15,7 +15,7 @@
# enable sending logs to syslog daemon
# public-key-file : string
# file with public key for
-# image verification
+# image verification
# mtd-blacklist : list integers
# MTD devices where SWUpdate
# must not try to check for UBI filesystem.
@@ -120,7 +120,7 @@ identify : (
{ name = "swCompatibility"; value = "unknown";}
);

-#
+#
# suricatta section: setup for backend
#
# Currently, they refer to the Hawkbit agent.

@@ -169,6 +169,10 @@ identify : (

# initial-report-resend-period : integer
# Specify period between re-tryint to send initial state, specified with "-c" option,
# when connection to Hawkbit is not available. Default value is 10 seconds.
+# connection-timeout : integer
+# Specify server connection timeout. If no connection has been established in this
+# period, libcurl will consider connection unsuccessful.

+# Default value is determined by underlying libcurl implementation (300s).

suricatta :
{
@@ -184,6 +188,7 @@ suricatta :

groupid = 1000;
enable = true;
initial-report-resend-period = 30;
+ connection-timeout = 10;

/*
cafile = "/etc/ssl/cafile";
sslkey = "/etc/ssl/sslkey";
diff --git a/include/channel_curl.h b/include/channel_curl.h

index 67cc971..4de098e 100644
--- a/include/channel_curl.h
+++ b/include/channel_curl.h
@@ -60,6 +60,7 @@ typedef struct {

unsigned int method;
unsigned int retries;
unsigned int low_speed_timeout;
+ unsigned int connection_timeout;
channel_body_t format;
bool debug;
bool usessl;
diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c

index f0d10c0..33d2eec 100644

--- a/suricatta/server_hawkbit.c
+++ b/suricatta/server_hawkbit.c
@@ -51,6 +51,7 @@ static struct option long_options[] = {
{"disable-token-for-dwl", no_argument, NULL, '1'},
{"cache", required_argument, NULL, '2'},
{"initial-report-resend-period", required_argument, NULL, 'm'},
+ {"connection-timeout", required_argument, NULL, 's'},
{NULL, 0, NULL, 0}};

static unsigned short mandatory_argument_count = 0;
@@ -130,7 +131,8 @@ static channel_data_t channel_data_defaults = {.debug = false,
.format = CHANNEL_PARSE_JSON,
.nocheckanswer = false,
.nofollow = false,
- .strictssl = true
+ .strictssl = true,
+ .connection_timeout = 0
};

static struct timeval server_time;

@@ -1599,7 +1601,8 @@ void server_print_help(void)

"\t --disable-token-for-dwl Do not send authentication header when downlloading SWU.\n"
"\t --cache <file> Use cache file as starting SWU\n"
"\t -m, --initial-report-resend-period <seconds> Time to wait prior to retry "
- "sending initial state with '-c' option (default: %ds).\n",
+ "sending initial state with '-c' option (default: %ds).\n"

+ "\t -s, --connection-timeout Set the server connection timeout (default: 300s).\n",
CHANNEL_DEFAULT_POLLING_INTERVAL, CHANNEL_DEFAULT_RESUME_TRIES,
CHANNEL_DEFAULT_RESUME_DELAY,
INITIAL_STATUS_REPORT_WAIT_DELAY);
@@ -1636,6 +1639,9 @@ static int server_hawkbit_settings(void *elem, void __attribute__ ((__unused__)

get_field(LIBCFG_PARSER, elem, "usetokentodwl",
&server_hawkbit.usetokentodwl);

+ get_field(LIBCFG_PARSER, elem, "connection-timeout",
+ &channel_data_defaults.connection_timeout);
+
GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "targettoken", tmp);
if (strlen(tmp))
SETSTRING(server_hawkbit.targettoken, tmp);

@@ -1683,7 +1689,7 @@ server_op_res_t server_start(char *fname, int argc, char *argv[])

/* reset to optind=1 to parse suricatta's argument vector */
optind = 1;
opterr = 0;
- while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:g:f:2:m:",
+ while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:g:f:2:m:s:",
long_options, NULL)) != -1) {
switch (choice) {
case 't':

@@ -1772,6 +1778,10 @@ server_op_res_t server_start(char *fname, int argc, char *argv[])

[Stefano Babic]

Hi Sava,

I get several conflicts by merging, maybe due to last inclusions. Can
you take a look and rebase on current -master ? Thanks !

Best regards,
Stefano Babic

[Sava Jakovljev]

@@ -643,6 +643,15 @@ Mandatory arguments are marked with '\*':
| | | Suricatta is started with initial state of |
| | | STATE_WAIT ("-c 6"), this value is ignored.|
+-------------------------+----------+--------------------------------------------+
+| -s <seconds> | integer | Connection timeout to use in seconds. |
+| | | If user doesn't set this option, default |
+| | | libcurl connection timeout value of 300 |
+| | | seconds is used. |
+| | | NOTE: it is not possible for Suricatta to |
+| | | respond to external program API requests |
+| | | during this period - adapt this value to |
+| | | your use case! |
++-------------------------+----------+--------------------------------------------+


systemd Integration

diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg
index c0547c5..2e12e15 100644
--- a/examples/configuration/swupdate.cfg
+++ b/examples/configuration/swupdate.cfg

2.30.2

[Stefano Babic]

Hi Sava,

Applied to -master, thanks !

Best regards,
Stefano Babic