| Message ID | 20240530082007.1427631-5-dominique.martinet@atmark-techno.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | downloader curl options: add max-download-speed | expand |
On 30.05.24 10:20, Dominique Martinet wrote: > Many callers of ustrtoull were not checking for errors. > Since there are legacy SWUs that might depend on them, many of these > place should not start erroring out immediately, but we must start > somewhere: > - add many warnings when ustrtoull failed, without impacting the > final result > - for the ones that are the most user facing (command line arguments), > make it a hard error > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > v3: new patch. > > Happy to split this up if you prefer; adding warnings probably shouldn't > hurt anything but the errors might be better split out. > > corelib/server_utils.c | 6 +++++- > handlers/copy_handler.c | 10 ++++++++-- > handlers/delta_handler.c | 7 +++++-- > handlers/diskpart_handler.c | 4 ++++ > suricatta/server_general.c | 5 +++++ > suricatta/server_hawkbit.c | 8 ++++++++ > suricatta/server_lua.c | 3 +++ > 7 files changed, 38 insertions(+), 5 deletions(-) > > diff --git a/corelib/server_utils.c b/corelib/server_utils.c > index 2b3c64e3ad0a..9c9265e2f2ca 100644 > --- a/corelib/server_utils.c > +++ b/corelib/server_utils.c > @@ -4,6 +4,7 @@ > * > * SPDX-License-Identifier: GPL-2.0-only > */ > +#include <errno.h> > #include <unistd.h> > #include <stdbool.h> > #include <stdlib.h> > @@ -24,8 +25,11 @@ int channel_settings(void *elem, void *data) > &chan->retries); > > GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "max-download-speed", tmp); > - if (strlen(tmp)) > + if (strlen(tmp)) { > chan->max_download_speed = (unsigned int)ustrtoull(tmp, NULL, 10); > + if (errno) > + WARN("max-download-speed setting %s: ustrtoull failed", tmp); > + } > > GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "retrywait", tmp); > if (strlen(tmp)) > diff --git a/handlers/copy_handler.c b/handlers/copy_handler.c > index abf65bd35e2e..3e2f1f40f9e5 100644 > --- a/handlers/copy_handler.c > +++ b/handlers/copy_handler.c > @@ -204,9 +204,10 @@ static int recurse_directory(const char *fpath, const struct stat *sb, > static int copy_image_file(struct img_type *img, void *data) > { > int ret = 0; > + char *tmp; > struct dict_list *proplist; > struct dict_list_elem *entry; > - size_t size; > + size_t size = 0; > struct script_handler_data *script_data; > bool recursive, createdest; > > @@ -249,7 +250,12 @@ static int copy_image_file(struct img_type *img, void *data) > /* > * Detect the size if not set in sw-descriptiont > */ > - size = dict_get_value(&img->properties, "size") ? ustrtoull(dict_get_value(&img->properties, "size"), NULL, 0) : 0; > + tmp = dict_get_value(&img->properties, "size"); > + if (tmp) { > + size = ustrtoull(tmp, NULL, 0); > + if (errno) > + WARN("size property %s: ustrtoull failed", tmp); > + } > > /* > * No chain set, fallback to rawcopy > diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c > index 89160036ed87..8e7faa0ead02 100644 > --- a/handlers/delta_handler.c > +++ b/handlers/delta_handler.c > @@ -326,10 +326,13 @@ static int delta_retrieve_attributes(struct img_type *img, struct hnd_priv *priv > char *srcsize; > srcsize = dict_get_value(&img->properties, "source-size"); > if (srcsize) { > - if (!strcmp(srcsize, "detect")) > + if (!strcmp(srcsize, "detect")) { > priv->detectsrcsize = true; > - else > + } else { > priv->srcsize = ustrtoull(srcsize, NULL, 10); > + if (errno) > + WARN("source-size %s: ustrotull failed", srcsize); > + } > } > > char *zckloglevel = dict_get_value(&img->properties, "zckloglevel"); > diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c > index 4c2b902b5b7a..982c494e2b27 100644 > --- a/handlers/diskpart_handler.c > +++ b/handlers/diskpart_handler.c > @@ -1232,11 +1232,15 @@ static int diskpart(struct img_type *img, > switch (i) { > case PART_SIZE: > part->size = ustrtoull(equal, NULL, 10); > + if (errno) > + WARN("partition size %s: ustrtoull failed", equal); > if (!size_delimiter_match(equal)) > part->explicit_size = 1; > break; > case PART_START: > part->start = ustrtoull(equal, NULL, 10); > + if (errno) > + WARN("partition size %s: ustrtoull failed", equal); > break; > case PART_TYPE: > strncpy(part->type, equal, sizeof(part->type)); > diff --git a/suricatta/server_general.c b/suricatta/server_general.c > index 5c51fda698c4..db5a60aec090 100644 > --- a/suricatta/server_general.c > +++ b/suricatta/server_general.c > @@ -663,6 +663,11 @@ static server_op_res_t server_start(const char *fname, int argc, char *argv[]) > case 'n': > channel_data_defaults.max_download_speed = > (unsigned int)ustrtoull(optarg, NULL, 10); > + if (errno) { > + ERROR("max-download-speed %s: ustrtoull failed", > + optarg); > + return SERVER_EINIT; > + } > break; > > /* Ignore the --server option which is already parsed by the caller. */ > diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c > index 3c38ea61bdf6..9c3e97418361 100644 > --- a/suricatta/server_hawkbit.c > +++ b/suricatta/server_hawkbit.c > @@ -871,6 +871,9 @@ static void get_action_id_from_env(int *action_id) > char *action_str = swupdate_vars_get("action_id", NULL); > if (action_str) { > int tmp = ustrtoull(action_str, NULL, 10); > + if (errno) > + WARN("action_id %s: ustrtoull failed", > + action_str); > /* > * action_id = 0 is invalid, then check it > */ > @@ -1910,6 +1913,11 @@ static server_op_res_t server_start(const char *fname, int argc, char *argv[]) > case 'n': > channel_data_defaults.max_download_speed = > (unsigned int)ustrtoull(optarg, NULL, 10); > + if (errno) { > + ERROR("max-download-speed %s: ustrtoull failed", > + optarg); > + return SERVER_EINIT; > + } > break; > /* Ignore not recognized options, they can be already parsed by the caller */ > case '?': > diff --git a/suricatta/server_lua.c b/suricatta/server_lua.c > index f5b90f6d5abc..c749c1c5de36 100644 > --- a/suricatta/server_lua.c > +++ b/suricatta/server_lua.c > @@ -591,6 +591,9 @@ static void channel_set_options(lua_State *L, channel_data_t *channel_data) > get_from_table(L, "max_download_speed", max_download_speed); > if (max_download_speed) { > channel_data->max_download_speed = (unsigned int)ustrtoull(max_download_speed, NULL, 10); > + if (errno) > + WARN("max-download-speed %s: ustrtoull failed", > + max_download_speed); > free(max_download_speed); > } > lua_getfield(L, -1, "proxy"); Thanks for this cleanup. Acked-by: Stefano Babic <stefano.babic@swupdate.org>
diff --git a/corelib/server_utils.c b/corelib/server_utils.c index 2b3c64e3ad0a..9c9265e2f2ca 100644 --- a/corelib/server_utils.c +++ b/corelib/server_utils.c @@ -4,6 +4,7 @@ * * SPDX-License-Identifier: GPL-2.0-only */ +#include <errno.h> #include <unistd.h> #include <stdbool.h> #include <stdlib.h> @@ -24,8 +25,11 @@ int channel_settings(void *elem, void *data) &chan->retries); GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "max-download-speed", tmp); - if (strlen(tmp)) + if (strlen(tmp)) { chan->max_download_speed = (unsigned int)ustrtoull(tmp, NULL, 10); + if (errno) + WARN("max-download-speed setting %s: ustrtoull failed", tmp); + } GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "retrywait", tmp); if (strlen(tmp)) diff --git a/handlers/copy_handler.c b/handlers/copy_handler.c index abf65bd35e2e..3e2f1f40f9e5 100644 --- a/handlers/copy_handler.c +++ b/handlers/copy_handler.c @@ -204,9 +204,10 @@ static int recurse_directory(const char *fpath, const struct stat *sb, static int copy_image_file(struct img_type *img, void *data) { int ret = 0; + char *tmp; struct dict_list *proplist; struct dict_list_elem *entry; - size_t size; + size_t size = 0; struct script_handler_data *script_data; bool recursive, createdest; @@ -249,7 +250,12 @@ static int copy_image_file(struct img_type *img, void *data) /* * Detect the size if not set in sw-descriptiont */ - size = dict_get_value(&img->properties, "size") ? ustrtoull(dict_get_value(&img->properties, "size"), NULL, 0) : 0; + tmp = dict_get_value(&img->properties, "size"); + if (tmp) { + size = ustrtoull(tmp, NULL, 0); + if (errno) + WARN("size property %s: ustrtoull failed", tmp); + } /* * No chain set, fallback to rawcopy diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c index 89160036ed87..8e7faa0ead02 100644 --- a/handlers/delta_handler.c +++ b/handlers/delta_handler.c @@ -326,10 +326,13 @@ static int delta_retrieve_attributes(struct img_type *img, struct hnd_priv *priv char *srcsize; srcsize = dict_get_value(&img->properties, "source-size"); if (srcsize) { - if (!strcmp(srcsize, "detect")) + if (!strcmp(srcsize, "detect")) { priv->detectsrcsize = true; - else + } else { priv->srcsize = ustrtoull(srcsize, NULL, 10); + if (errno) + WARN("source-size %s: ustrotull failed", srcsize); + } } char *zckloglevel = dict_get_value(&img->properties, "zckloglevel"); diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c index 4c2b902b5b7a..982c494e2b27 100644 --- a/handlers/diskpart_handler.c +++ b/handlers/diskpart_handler.c @@ -1232,11 +1232,15 @@ static int diskpart(struct img_type *img, switch (i) { case PART_SIZE: part->size = ustrtoull(equal, NULL, 10); + if (errno) + WARN("partition size %s: ustrtoull failed", equal); if (!size_delimiter_match(equal)) part->explicit_size = 1; break; case PART_START: part->start = ustrtoull(equal, NULL, 10); + if (errno) + WARN("partition size %s: ustrtoull failed", equal); break; case PART_TYPE: strncpy(part->type, equal, sizeof(part->type)); diff --git a/suricatta/server_general.c b/suricatta/server_general.c index 5c51fda698c4..db5a60aec090 100644 --- a/suricatta/server_general.c +++ b/suricatta/server_general.c @@ -663,6 +663,11 @@ static server_op_res_t server_start(const char *fname, int argc, char *argv[]) case 'n': channel_data_defaults.max_download_speed = (unsigned int)ustrtoull(optarg, NULL, 10); + if (errno) { + ERROR("max-download-speed %s: ustrtoull failed", + optarg); + return SERVER_EINIT; + } break; /* Ignore the --server option which is already parsed by the caller. */ diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c index 3c38ea61bdf6..9c3e97418361 100644 --- a/suricatta/server_hawkbit.c +++ b/suricatta/server_hawkbit.c @@ -871,6 +871,9 @@ static void get_action_id_from_env(int *action_id) char *action_str = swupdate_vars_get("action_id", NULL); if (action_str) { int tmp = ustrtoull(action_str, NULL, 10); + if (errno) + WARN("action_id %s: ustrtoull failed", + action_str); /* * action_id = 0 is invalid, then check it */ @@ -1910,6 +1913,11 @@ static server_op_res_t server_start(const char *fname, int argc, char *argv[]) case 'n': channel_data_defaults.max_download_speed = (unsigned int)ustrtoull(optarg, NULL, 10); + if (errno) { + ERROR("max-download-speed %s: ustrtoull failed", + optarg); + return SERVER_EINIT; + } break; /* Ignore not recognized options, they can be already parsed by the caller */ case '?': diff --git a/suricatta/server_lua.c b/suricatta/server_lua.c index f5b90f6d5abc..c749c1c5de36 100644 --- a/suricatta/server_lua.c +++ b/suricatta/server_lua.c @@ -591,6 +591,9 @@ static void channel_set_options(lua_State *L, channel_data_t *channel_data) get_from_table(L, "max_download_speed", max_download_speed); if (max_download_speed) { channel_data->max_download_speed = (unsigned int)ustrtoull(max_download_speed, NULL, 10); + if (errno) + WARN("max-download-speed %s: ustrtoull failed", + max_download_speed); free(max_download_speed); } lua_getfield(L, -1, "proxy");
Many callers of ustrtoull were not checking for errors. Since there are legacy SWUs that might depend on them, many of these place should not start erroring out immediately, but we must start somewhere: - add many warnings when ustrtoull failed, without impacting the final result - for the ones that are the most user facing (command line arguments), make it a hard error Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- v3: new patch. Happy to split this up if you prefer; adding warnings probably shouldn't hurt anything but the errors might be better split out. corelib/server_utils.c | 6 +++++- handlers/copy_handler.c | 10 ++++++++-- handlers/delta_handler.c | 7 +++++-- handlers/diskpart_handler.c | 4 ++++ suricatta/server_general.c | 5 +++++ suricatta/server_hawkbit.c | 8 ++++++++ suricatta/server_lua.c | 3 +++ 7 files changed, 38 insertions(+), 5 deletions(-)