| Message ID | 20210421162851.1027886-1-sava.jakovljev@teufel.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | libcurl channel: enable connection timeout option | expand |
I submitted same patch as before, but this without missing Signed-Off. Best regards, Sava Jakovljev Sava Jakovljev schrieb am Mittwoch, 21. April 2021 um 18:29:15 UTC+2: > * 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 | 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 > >
Hi Sava, On 21.04.21 18:28, Sava Jakovljev wrote: > * Add new configuration option for Suricatta which enables users to set > connection timeout in seconds. > * Update documentation. > > Signed-off-by: Sava Jakovljev <sava.jakovljev@teufel.de> > --- > 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) || 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. > (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! | > ++-------------------------+----------+--------------------------------------------+ 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. > > > 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; What is this 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 > Best regards, Stefano Babic
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 schrieb am Freitag, 23. April 2021 um 10:06:16 UTC+2: > Hi Sava, > > On 21.04.21 18:28, Sava Jakovljev wrote: > > * 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 | 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) || > > 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. > > > (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! | > > > ++-------------------------+----------+--------------------------------------------+ > > 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. > > > > > > > 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; > > What is this 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 > > > > 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 <+49%208142%206698953> Fax: +49-8142-66989-80 > <+49%208142%206698980> Email: sba...@denx.de > ===================================================================== >
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 > > Best regards, > Sava Jakovljev > > > Stefano Babic schrieb am Freitag, 23. April 2021 um 10:06:16 UTC+2: > > Hi Sava, > > On 21.04.21 18:28, Sava Jakovljev wrote: > > * 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 | 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) || > > 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. > > > (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! | > > > ++-------------------------+----------+--------------------------------------------+ > > > 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. > > > > > > > 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; > > What is this 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 > > > > 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 <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+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@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>.
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 Stefano Babic schrieb am Freitag, 23. April 2021 um 10:51:37 UTC+2: > 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 > > > > > Best regards, > > Sava Jakovljev > > > > > > Stefano Babic schrieb am Freitag, 23. April 2021 um 10:06:16 UTC+2: > > > > Hi Sava, > > > > On 21.04.21 18:28, Sava Jakovljev wrote: > > > * 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 | 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) || > > > > 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. > > > > > (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! | > > > > > > ++-------------------------+----------+--------------------------------------------+ > > > > > > 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. > > > > > > > > > > > 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; > > > > What is this 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 > > > > > > > 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 <+49%208142%206698953> > <tel:+49%208142%206698953> Fax: > > +49-8142-66989-80 <+49%208142%206698980> <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 > >. > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <+49%208142%206698953> Fax: +49-8142-66989-80 > <+49%208142%206698980> Email: sba...@denx.de > ===================================================================== >
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;
* Add new configuration option for Suricatta which enables users to set connection timeout in seconds. * Update documentation. Signed-off-by: Sava Jakovljev <sava.jakovljev@teufel.de> --- 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(-) -- 2.26.3