| Message ID | 20210126131412.3567-4-michael.adler@siemens.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Overhaul swupdate.cfg handling | expand |
On 26.01.21 14:14, Michael Adler wrote: > Previously, this option could only be specified using command-line > arguments or via the IPC interface. > > Signed-off-by: Michael Adler <michael.adler@siemens.com> > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > core/swupdate.c | 12 +++++++++++- > examples/configuration/swupdate.cfg | 2 ++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/core/swupdate.c b/core/swupdate.c > index 788981e..ff3cd16 100644 > --- a/core/swupdate.c > +++ b/core/swupdate.c > @@ -363,6 +363,12 @@ static int read_globals_settings(void *elem, void *data) > GET_FIELD_STRING(LIBCFG_PARSER, elem, "forced-signer-name", > sw->globals.forced_signer_name); > > + char software_select[SWUPDATE_GENERAL_STRING_SIZE] = ""; > + GET_FIELD_STRING(LIBCFG_PARSER, elem, "select", software_select); > + if (parse_image_selector(software_select, sw)) { > + return 1; > + } > + Have you tested without setting a select in config ? IMHO according the sources, software_select is an empty string, and parse_image_selector() returns with error because it cannot find the "," delimiter. That is, an entry in swupdate.cfg becomes mandatory (this is not acceptable). > return 0; > } > > @@ -809,8 +815,12 @@ int main(int argc, char **argv) > fprintf(stderr, "Error: Incorrect select option format.\n"); > exit(EXIT_FAILURE); > } > + } > + > + > + if (strlen(swcfg.globals.default_software_set) > 0 && strlen(swcfg.globals.default_running_mode) > 0) { > INFO("software set: %s mode: %s", swcfg.globals.default_software_set, > - swcfg.globals.default_running_mode); > + swcfg.globals.default_running_mode); which is the change here ? > } > > /* Read sw-versions */ > diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg > index 6fad156..a97f982 100644 > --- a/examples/configuration/swupdate.cfg > +++ b/examples/configuration/swupdate.cfg > @@ -38,6 +38,8 @@ > # [emailProtection|codeSigning] (default: emailProtection) > # forced-signer-name : string > # set expected common name of signer certificate > +# select: : string > +# select software images set and source (<software>,<mode>) > globals : > { > > Best regards, Stefano
Hi Stefano, > Have you tested without setting a select in config ? IMHO according the sources, software_select is an empty string, > and parse_image_selector() returns with error because it cannot find the "," delimiter. yes, parse_image_selector returns an error, but now things get interesting, because the error is ignored. > That is, an entry in swupdate.cfg becomes mandatory (this is not acceptable). 1) You are right, I should not call parse_image_selector if the string is empty because select is optional => I will fix this. 2) Nevertheless, the error is ignored and swupdate continues to start up correctly. The reason for 2) is the following line which is also present on master (swupdate_settings.c::read_module_settings) ``` fcn(elem, data); [...] return 0; ``` `fcn` is the settings callback, which returns an int, but the return value is ignored and consequently, any error of the settings callback is ignored. I'm suggesting the following fix for that: --- a/corelib/swupdate_settings.c +++ b/corelib/swupdate_settings.c @@ -176,7 +176,5 @@ int swupdate_cfg_read_module_settings(swupdate_cfg_handle *handle, const char *m return -ENODATA; } - fcn(elem, data); - - return 0; + return fcn(elem, data); } If you agree, I will add this to my v2 series as well. > which is the change here ? Only log the running_mode and software_set if it is non-empty. Kind regards, Michael
On 27.01.21 10:44, Michael Adler wrote: > Hi Stefano, > >> Have you tested without setting a select in config ? IMHO according the sources, software_select is an empty string, >> and parse_image_selector() returns with error because it cannot find the "," delimiter. > > yes, parse_image_selector returns an error, but now things get interesting, because the error is ignored. > >> That is, an entry in swupdate.cfg becomes mandatory (this is not acceptable). > > 1) You are right, I should not call parse_image_selector if the string is empty because select is optional => I will fix this. Ok > 2) Nevertheless, the error is ignored and swupdate continues to start up correctly. This is not in parse_selector() > > The reason for 2) is the following line which is also present on master (swupdate_settings.c::read_module_settings) > > ``` > fcn(elem, data); > > [...] > > return 0; > ``` This is how I designed this - all callbacks (check in code) are returning 0. That means: it is not mandatory that entries for each module are present into swupdate.cfg, and the syntax in swupdate.cfg is not checked, letting users to add own attribute if required. The signature for the callbacks could be changed to void fcn() instead of int fcn(). > > `fcn` is the settings callback, which returns an int, but the return value is ignored and consequently, any error of the > settings callback is ignored. If I have not introduced bugs, all callbacks should be consistent and return simply 0. A cleanup should be to change the return code to void. > I'm suggesting the following fix for that: > > --- a/corelib/swupdate_settings.c > +++ b/corelib/swupdate_settings.c > @@ -176,7 +176,5 @@ int swupdate_cfg_read_module_settings(swupdate_cfg_handle *handle, const char *m > return -ENODATA; > } > > - fcn(elem, data); > - > - return 0; > + return fcn(elem, data); > } This is also consistent, it should not change the behavior as I stated before. We could have a callback making some more mandatory checks in future. > > If you agree, I will add this to my v2 series as well. > It is fine. >> which is the change here ? > > Only log the running_mode and software_set if it is non-empty. > Best regards, Stefano
diff --git a/core/swupdate.c b/core/swupdate.c index 788981e..ff3cd16 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -363,6 +363,12 @@ static int read_globals_settings(void *elem, void *data) GET_FIELD_STRING(LIBCFG_PARSER, elem, "forced-signer-name", sw->globals.forced_signer_name); + char software_select[SWUPDATE_GENERAL_STRING_SIZE] = ""; + GET_FIELD_STRING(LIBCFG_PARSER, elem, "select", software_select); + if (parse_image_selector(software_select, sw)) { + return 1; + } + return 0; } @@ -809,8 +815,12 @@ int main(int argc, char **argv) fprintf(stderr, "Error: Incorrect select option format.\n"); exit(EXIT_FAILURE); } + } + + + if (strlen(swcfg.globals.default_software_set) > 0 && strlen(swcfg.globals.default_running_mode) > 0) { INFO("software set: %s mode: %s", swcfg.globals.default_software_set, - swcfg.globals.default_running_mode); + swcfg.globals.default_running_mode); } /* Read sw-versions */ diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg index 6fad156..a97f982 100644 --- a/examples/configuration/swupdate.cfg +++ b/examples/configuration/swupdate.cfg @@ -38,6 +38,8 @@ # [emailProtection|codeSigning] (default: emailProtection) # forced-signer-name : string # set expected common name of signer certificate +# select: : string +# select software images set and source (<software>,<mode>) globals : {