diff mbox series

[4/9] Config parser: Additional logging

Message ID 20210126131412.3567-5-michael.adler@siemens.com
State Changes Requested
Headers show
Series Overhaul swupdate.cfg handling | expand

Commit Message

Michael Adler Jan. 26, 2021, 1:14 p.m. UTC 
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(+)

Comments

Stefano Babic Jan. 26, 2021, 2:27 p.m. UTC | #1 
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
Michael Adler Jan. 26, 2021, 3:27 p.m. UTC | #2 
> 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
Stefano Babic Jan. 26, 2021, 3:44 p.m. UTC | #3 
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
Storm, Christian Jan. 26, 2021, 4:03 p.m. UTC | #4 
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
Stefano Babic Jan. 26, 2021, 4:04 p.m. UTC | #5 
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
>
Michael Adler Jan. 27, 2021, 8:12 a.m. UTC | #6 
> 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 mbox series

Patch

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);