Message ID | 20210126131412.3567-5-michael.adler@siemens.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Overhaul swupdate.cfg handling | expand |
On 26.01.21 14:14, Michael Adler wrote: > This makes it easier to see what's going on during the early start up of > SWUpdate. > > Signed-off-by: Michael Adler <michael.adler@siemens.com> > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > core/swupdate.c | 1 + > corelib/swupdate_settings.c | 3 +++ > parser/parser.c | 2 ++ > 3 files changed, 6 insertions(+) > > diff --git a/core/swupdate.c b/core/swupdate.c > index ff3cd16..6548d06 100644 > --- a/core/swupdate.c > +++ b/core/swupdate.c > @@ -217,6 +217,7 @@ static int parse_image_selector(const char *selector, struct swupdate_cfg *sw) > { > char *pos; > > + TRACE("Parsing image selector: %s", selector); > pos = strchr(selector, ','); > if (pos == NULL) > return -EINVAL; > diff --git a/corelib/swupdate_settings.c b/corelib/swupdate_settings.c > index b0d3344..de7f331 100644 > --- a/corelib/swupdate_settings.c > +++ b/corelib/swupdate_settings.c > @@ -53,6 +53,7 @@ static int read_settings_file(config_t *cfg, const char *filename) > { > int ret; > > + TRACE("Reading config file %s", filename); > ret = config_read_file(cfg, filename); > if (ret != CONFIG_TRUE) { > fprintf(stderr, "%s ", config_error_file(cfg)); > @@ -86,10 +87,12 @@ int read_module_settings(const char *filename, const char *module, settings_call > elem = find_settings_node(&cfg, module); > > if (!elem) { > + TRACE("No config settings found for module %s", module); > config_destroy(&cfg); > return -ENODATA; > } > > + TRACE("Reading config settings for module %s", module); > fcn(elem, data); > > config_destroy(&cfg); > diff --git a/parser/parser.c b/parser/parser.c > index c2fe175..9217028 100644 > --- a/parser/parser.c > +++ b/parser/parser.c > @@ -812,6 +812,7 @@ int parse_cfg (struct swupdate_cfg *swcfg, const char *filename) > config_init(&cfg); > > /* Read the file. If there is an error, report it and exit. */ > + TRACE("Parsing config file %s", filename); > if(config_read_file(&cfg, filename) != CONFIG_TRUE) { > printf("%s ", config_error_file(&cfg)); > printf("%d ", config_error_line(&cfg)); > @@ -851,6 +852,7 @@ int parse_json(struct swupdate_cfg *swcfg, const char *filename) > json_object *cfg; > parsertype p = JSON_PARSER; > > + TRACE("Parsing config file %s", filename); > /* Read the file. If there is an error, report it and exit. */ > ret = stat(filename, &stbuf); > > This is quite noisy - do we really need this ? It is just a function_entry trace. Best regards, Stefano
> This is quite noisy - do we really need this ? It is just a > function_entry trace. Yes, it's quite noisy indeed, but after all it is TRACE (which is somewhat expected to be noisy). It was useful for me to understand the whole parsing process, but I do not mind dropping this patch. I kind of expected this, so that's why I put the change into a separate commit. Your call :) Just let me know and I'll remove the comment in the next patch series. Kind regards, Michael
On 26.01.21 16:27, Michael Adler wrote: >> This is quite noisy - do we really need this ? It is just a >> function_entry trace. > > Yes, it's quite noisy indeed, but after all it is TRACE (which is somewhat expected to be noisy). > Then it should be DEBUG instead of TRACE. > It was useful for me to understand the whole parsing process, but I do not mind dropping this patch. I kind of expected > this, so that's why I put the change into a separate commit. > > Your call :) Just let me know and I'll remove the comment in the next patch series. I do not think this helps a lot - I suggest to drop it. Regards, Stefano
Hi Stefano, > >> This is quite noisy - do we really need this ? It is just a > >> function_entry trace. > > > > Yes, it's quite noisy indeed, but after all it is TRACE (which is > > somewhat expected to be noisy). > > > > Then it should be DEBUG instead of TRACE. > > > It was useful for me to understand the whole parsing process, but > > I do not mind dropping this patch. I kind of expected this, so > > that's why I put the change into a separate commit. > > > > Your call :) Just let me know and I'll remove the comment in the next patch series. > > I do not think this helps a lot - I suggest to drop it. Hm, I do think this is useful to actually get hinted on what is going on in the config parsing and in particular the sections that are read and applied. Actually more useful (to me) than, e.g., corelib/channel_curl.c's TRACE("Channel's effective URL resolved to %s", channel_curl->effective_url); But I'm with Michael here, your call... Kind regards, Christian
On 26.01.21 17:03, Christian Storm wrote: > Hi Stefano, > >>>> This is quite noisy - do we really need this ? It is just a >>>> function_entry trace. >>> >>> Yes, it's quite noisy indeed, but after all it is TRACE (which is >>> somewhat expected to be noisy). >>> >> >> Then it should be DEBUG instead of TRACE. >> >>> It was useful for me to understand the whole parsing process, but >>> I do not mind dropping this patch. I kind of expected this, so >>> that's why I put the change into a separate commit. >>> >>> Your call :) Just let me know and I'll remove the comment in the next patch series. >> >> I do not think this helps a lot - I suggest to drop it. > > Hm, I do think this is useful to actually get hinted on what is going on > in the config parsing and in particular the sections that are read and > applied. Actually more useful (to me) than, e.g., corelib/channel_curl.c's > TRACE("Channel's effective URL resolved to %s", > channel_curl->effective_url); > > But I'm with Michael here, your call... > If you change to DEBUG, I will apply it. Best regards, Stefano > > > Kind regards, > Christian >
> Then it should be DEBUG instead of TRACE. Good catch! I blindly assumed that TRACE is more verbose than DEBUG (which is the case in most logging frameworks, but after all, this is just a convention). > If you change to DEBUG, I will apply it. Great :) I'll change it and add it to the v2 series. Kind regards, Michael
diff --git a/core/swupdate.c b/core/swupdate.c index ff3cd16..6548d06 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -217,6 +217,7 @@ static int parse_image_selector(const char *selector, struct swupdate_cfg *sw) { char *pos; + TRACE("Parsing image selector: %s", selector); pos = strchr(selector, ','); if (pos == NULL) return -EINVAL; diff --git a/corelib/swupdate_settings.c b/corelib/swupdate_settings.c index b0d3344..de7f331 100644 --- a/corelib/swupdate_settings.c +++ b/corelib/swupdate_settings.c @@ -53,6 +53,7 @@ static int read_settings_file(config_t *cfg, const char *filename) { int ret; + TRACE("Reading config file %s", filename); ret = config_read_file(cfg, filename); if (ret != CONFIG_TRUE) { fprintf(stderr, "%s ", config_error_file(cfg)); @@ -86,10 +87,12 @@ int read_module_settings(const char *filename, const char *module, settings_call elem = find_settings_node(&cfg, module); if (!elem) { + TRACE("No config settings found for module %s", module); config_destroy(&cfg); return -ENODATA; } + TRACE("Reading config settings for module %s", module); fcn(elem, data); config_destroy(&cfg); diff --git a/parser/parser.c b/parser/parser.c index c2fe175..9217028 100644 --- a/parser/parser.c +++ b/parser/parser.c @@ -812,6 +812,7 @@ int parse_cfg (struct swupdate_cfg *swcfg, const char *filename) config_init(&cfg); /* Read the file. If there is an error, report it and exit. */ + TRACE("Parsing config file %s", filename); if(config_read_file(&cfg, filename) != CONFIG_TRUE) { printf("%s ", config_error_file(&cfg)); printf("%d ", config_error_line(&cfg)); @@ -851,6 +852,7 @@ int parse_json(struct swupdate_cfg *swcfg, const char *filename) json_object *cfg; parsertype p = JSON_PARSER; + TRACE("Parsing config file %s", filename); /* Read the file. If there is an error, report it and exit. */ ret = stat(filename, &stbuf);