diff mbox series

Fix handle of total downloaded bytes in progress

Message ID 20201109205713.380179-1-sbabic@denx.de
State Accepted
Headers show
Series Fix handle of total downloaded bytes in progress | expand

Commit Message

Stefano Babic Nov. 9, 2020, 8:57 p.m. UTC
commit 5738732 disables the number of downloaded bytes because buggy,
this fix the behavior and ensures that the value is correctly forwareded
to the listeners.

Signed-off-by: Stefano Babic <sbabic@denx.de>
---
 core/notifier.c           |  7 ++++---
 core/progress_thread.c    | 11 ++++++++---
 include/progress.h        |  9 ---------
 include/progress_ipc.h    |  1 +
 tools/swupdate-progress.c |  4 ++--
 5 files changed, 15 insertions(+), 17 deletions(-)

Comments

Christian Storm Nov. 18, 2020, 4:06 p.m. UTC | #1
Hi Stefano,

> commit 5738732 disables the number of downloaded bytes because buggy,
> this fix the behavior and ensures that the value is correctly forwareded
> to the listeners.
> 
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> ---
>  core/notifier.c           |  7 ++++---
>  core/progress_thread.c    | 11 ++++++++---
>  include/progress.h        |  9 ---------
>  include/progress_ipc.h    |  1 +
>  tools/swupdate-progress.c |  4 ++--
>  5 files changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/core/notifier.c b/core/notifier.c
> index 276e155..e4ca3c6 100644
> --- a/core/notifier.c
> +++ b/core/notifier.c
> @@ -339,15 +339,16 @@ static void process_notifier (RECOVERY_STATUS status, int event, int level, cons
>   */
>  static void progress_notifier (RECOVERY_STATUS status, int event, int level, const char *msg)
>  {
> +	int dwl_percent = 0;
> +	unsigned long long dwl_bytes = 0;
>  	(void)level;
>  
>  	/* Check just in case a process want to send an info outside */
>  	if (status != PROGRESS)
>  	       return;
>  
> -	if (event == RECOVERY_DWL) {
> -		struct progress_dwl_data *pdwl = (struct progress_dwl_data *)msg;
> -		swupdate_download_update(pdwl->dwl_percent, pdwl->dwl_bytes);
> +	if (event == RECOVERY_DWL && (sscanf(msg, "%d-%llu", &dwl_percent, &dwl_bytes) == 2)) {
> +		swupdate_download_update(dwl_percent, dwl_bytes);
>  		return;
>  	}
>  
> diff --git a/core/progress_thread.c b/core/progress_thread.c
> index cc556c1..7c424fc 100644
> --- a/core/progress_thread.c
> +++ b/core/progress_thread.c
> @@ -97,7 +97,7 @@ static void _swupdate_download_update(unsigned int perc, unsigned long long tota
>  	pthread_mutex_lock(&pprog->lock);
>  	if (perc != pprog->msg.dwl_percent) {
>  		pprog->msg.dwl_percent = perc;
> -		totalbytes = totalbytes;
> +		pprog->msg.dwl_bytes = totalbytes;
>  		send_progress_msg();
>  	}
>  	pthread_mutex_unlock(&pprog->lock);
> @@ -138,8 +138,13 @@ void swupdate_download_update(unsigned int perc, unsigned long long totalbytes)
>  	 * Not called by main process, for example by suricatta or Webserver
>  	 */
>  	if (pid == getpid()) {
> -		struct progress_dwl_data *pdwl = (struct progress_dwl_data *)info;
> -		pdwl->dwl_percent = perc;
> +		/*
> +		 * Notify can just receive a string as message
> +		 * so it is necessary to encode further information as string
> +		 * and decode them in the notifier, in this case
> +		 * the progress_notifier
> +		 */
> +		snprintf(info, sizeof(info) - 1, "%d-%llu", perc, totalbytes);
>  		notify(PROGRESS, RECOVERY_DWL, TRACELEVEL, info);
>  		return;
>  	}
> diff --git a/include/progress.h b/include/progress.h
> index 738e59f..9297cf9 100644
> --- a/include/progress.h
> +++ b/include/progress.h
> @@ -11,15 +11,6 @@
>  #include <swupdate_status.h>
>  #include <progress_ipc.h>
>  
> -/*
> - * Internal structures to be used to forward progress data
> - */
> -
> -struct progress_dwl_data {
> -	unsigned int	dwl_percent;	/* % downloaded data */
> -	unsigned long long dwl_bytes;	/* total of bytes to be downloaded */
> -};
> -
>  /*
>   * Internal SWUpdate functions to drive the progress
>   * interface. Common progress definitions for internal
> diff --git a/include/progress_ipc.h b/include/progress_ipc.h
> index f508eda..2efa61d 100644
> --- a/include/progress_ipc.h
> +++ b/include/progress_ipc.h
> @@ -27,6 +27,7 @@ struct progress_msg {
>  	unsigned int	magic;		/* Magic Number */
>  	RECOVERY_STATUS	status;		/* Update Status (Running, Failure) */
>  	unsigned int	dwl_percent;	/* % downloaded data */
> +	unsigned long long dwl_bytes;   /* total of bytes to be downloaded */
>  	unsigned int	nsteps;		/* No. total of steps */
>  	unsigned int	cur_step;	/* Current step index */
>  	unsigned int	cur_percent;	/* % in current step */
> diff --git a/tools/swupdate-progress.c b/tools/swupdate-progress.c
> index ecc0b40..754d56f 100644
> --- a/tools/swupdate-progress.c
> +++ b/tools/swupdate-progress.c
> @@ -327,11 +327,11 @@ int main(int argc, char **argv)
>  				fill_progress_bar(bar, sizeof(bar), msg.cur_percent);
>  
>  				if (!silent) {
> -					fprintf(stdout, "[ %.*s ] %d of %d %d%% (%s) dwl %d%%\r",
> +					fprintf(stdout, "[ %.*s ] %d of %d %d%% (%s), dwl %d%% of %llu bytes\r",
>  						bar_len,
>  						bar,
>  						msg.cur_step, msg.nsteps, msg.cur_percent,
> -						msg.cur_image, msg.dwl_percent);
> +						msg.cur_image, msg.dwl_percent, msg.dwl_bytes);
>  					fflush(stdout);
>  				}
>  
> -- 
> 2.25.1
> 

Just found that this breaks core/syslog.c by it emitting messages like
  FATAL_UNKNOWN 15-217716224
with the message being the <percent>-<filesize> encoding.

The 'FATAL_' prefix stems from this line in core/syslog.c
  syslog(logprio, "%s%s %s\n", ((error != (int)RECOVERY_NO_ERROR) ? "FATAL_" : ""), statusMsg, msg);
where error == RECOVERY_DWL should also not result in the 'FATAL_' prefix.

Then, 
  switch(status) {
should also cover 
  case PROGRESS: statusMsg = "PROGRESS"; break;
and probably also SUBPROCESS for completeness.

This silences the FATAL alarm but still you get logged the encoded
information in <percent>-<filesize> syntax. IMO, it should print
something more friendly to human interpretation :)



Kind regards,
   Christian
Stefano Babic Nov. 18, 2020, 4:12 p.m. UTC | #2
Hi Christian,

On 18.11.20 17:06, Christian Storm wrote:
> Hi Stefano,
> 
>> commit 5738732 disables the number of downloaded bytes because buggy,
>> this fix the behavior and ensures that the value is correctly forwareded
>> to the listeners.
>>
>> Signed-off-by: Stefano Babic <sbabic@denx.de>
>> ---
>>  core/notifier.c           |  7 ++++---
>>  core/progress_thread.c    | 11 ++++++++---
>>  include/progress.h        |  9 ---------
>>  include/progress_ipc.h    |  1 +
>>  tools/swupdate-progress.c |  4 ++--
>>  5 files changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/core/notifier.c b/core/notifier.c
>> index 276e155..e4ca3c6 100644
>> --- a/core/notifier.c
>> +++ b/core/notifier.c
>> @@ -339,15 +339,16 @@ static void process_notifier (RECOVERY_STATUS status, int event, int level, cons
>>   */
>>  static void progress_notifier (RECOVERY_STATUS status, int event, int level, const char *msg)
>>  {
>> +	int dwl_percent = 0;
>> +	unsigned long long dwl_bytes = 0;
>>  	(void)level;
>>  
>>  	/* Check just in case a process want to send an info outside */
>>  	if (status != PROGRESS)
>>  	       return;
>>  
>> -	if (event == RECOVERY_DWL) {
>> -		struct progress_dwl_data *pdwl = (struct progress_dwl_data *)msg;
>> -		swupdate_download_update(pdwl->dwl_percent, pdwl->dwl_bytes);
>> +	if (event == RECOVERY_DWL && (sscanf(msg, "%d-%llu", &dwl_percent, &dwl_bytes) == 2)) {
>> +		swupdate_download_update(dwl_percent, dwl_bytes);
>>  		return;
>>  	}
>>  
>> diff --git a/core/progress_thread.c b/core/progress_thread.c
>> index cc556c1..7c424fc 100644
>> --- a/core/progress_thread.c
>> +++ b/core/progress_thread.c
>> @@ -97,7 +97,7 @@ static void _swupdate_download_update(unsigned int perc, unsigned long long tota
>>  	pthread_mutex_lock(&pprog->lock);
>>  	if (perc != pprog->msg.dwl_percent) {
>>  		pprog->msg.dwl_percent = perc;
>> -		totalbytes = totalbytes;
>> +		pprog->msg.dwl_bytes = totalbytes;
>>  		send_progress_msg();
>>  	}
>>  	pthread_mutex_unlock(&pprog->lock);
>> @@ -138,8 +138,13 @@ void swupdate_download_update(unsigned int perc, unsigned long long totalbytes)
>>  	 * Not called by main process, for example by suricatta or Webserver
>>  	 */
>>  	if (pid == getpid()) {
>> -		struct progress_dwl_data *pdwl = (struct progress_dwl_data *)info;
>> -		pdwl->dwl_percent = perc;
>> +		/*
>> +		 * Notify can just receive a string as message
>> +		 * so it is necessary to encode further information as string
>> +		 * and decode them in the notifier, in this case
>> +		 * the progress_notifier
>> +		 */
>> +		snprintf(info, sizeof(info) - 1, "%d-%llu", perc, totalbytes);
>>  		notify(PROGRESS, RECOVERY_DWL, TRACELEVEL, info);
>>  		return;
>>  	}
>> diff --git a/include/progress.h b/include/progress.h
>> index 738e59f..9297cf9 100644
>> --- a/include/progress.h
>> +++ b/include/progress.h
>> @@ -11,15 +11,6 @@
>>  #include <swupdate_status.h>
>>  #include <progress_ipc.h>
>>  
>> -/*
>> - * Internal structures to be used to forward progress data
>> - */
>> -
>> -struct progress_dwl_data {
>> -	unsigned int	dwl_percent;	/* % downloaded data */
>> -	unsigned long long dwl_bytes;	/* total of bytes to be downloaded */
>> -};
>> -
>>  /*
>>   * Internal SWUpdate functions to drive the progress
>>   * interface. Common progress definitions for internal
>> diff --git a/include/progress_ipc.h b/include/progress_ipc.h
>> index f508eda..2efa61d 100644
>> --- a/include/progress_ipc.h
>> +++ b/include/progress_ipc.h
>> @@ -27,6 +27,7 @@ struct progress_msg {
>>  	unsigned int	magic;		/* Magic Number */
>>  	RECOVERY_STATUS	status;		/* Update Status (Running, Failure) */
>>  	unsigned int	dwl_percent;	/* % downloaded data */
>> +	unsigned long long dwl_bytes;   /* total of bytes to be downloaded */
>>  	unsigned int	nsteps;		/* No. total of steps */
>>  	unsigned int	cur_step;	/* Current step index */
>>  	unsigned int	cur_percent;	/* % in current step */
>> diff --git a/tools/swupdate-progress.c b/tools/swupdate-progress.c
>> index ecc0b40..754d56f 100644
>> --- a/tools/swupdate-progress.c
>> +++ b/tools/swupdate-progress.c
>> @@ -327,11 +327,11 @@ int main(int argc, char **argv)
>>  				fill_progress_bar(bar, sizeof(bar), msg.cur_percent);
>>  
>>  				if (!silent) {
>> -					fprintf(stdout, "[ %.*s ] %d of %d %d%% (%s) dwl %d%%\r",
>> +					fprintf(stdout, "[ %.*s ] %d of %d %d%% (%s), dwl %d%% of %llu bytes\r",
>>  						bar_len,
>>  						bar,
>>  						msg.cur_step, msg.nsteps, msg.cur_percent,
>> -						msg.cur_image, msg.dwl_percent);
>> +						msg.cur_image, msg.dwl_percent, msg.dwl_bytes);
>>  					fflush(stdout);
>>  				}
>>  
>> -- 
>> 2.25.1
>>
> 
> Just found that this breaks core/syslog.c by it emitting messages like
>   FATAL_UNKNOWN 15-217716224
> with the message being the <percent>-<filesize> encoding.
> 
> The 'FATAL_' prefix stems from this line in core/syslog.c
>   syslog(logprio, "%s%s %s\n", ((error != (int)RECOVERY_NO_ERROR) ? "FATAL_" : ""), statusMsg, msg);
> where error == RECOVERY_DWL should also not result in the 'FATAL_' prefix.
> 
> Then, 
>   switch(status) {
> should also cover 
>   case PROGRESS: statusMsg = "PROGRESS"; break;
> and probably also SUBPROCESS for completeness.
> 
> This silences the FATAL alarm but still you get logged the encoded
> information in <percent>-<filesize> syntax. IMO, it should print
> something more friendly to human interpretation :)

Well, but this generates an overflow of dummy messages for syslog
without meaning, as the message is addressed to the progress interface
instead of the logging. I will tend to not log unknown sources instead
ofadding them. So like:

      case DONE: statusMsg = "DONE"; break;
      default:
	return;

Best regards,
Stefano
Christian Storm Nov. 18, 2020, 5:04 p.m. UTC | #3
Hi Stefano,

> >> commit 5738732 disables the number of downloaded bytes because buggy,
> >> this fix the behavior and ensures that the value is correctly forwareded
> >> to the listeners.
> >>
> >> Signed-off-by: Stefano Babic <sbabic@denx.de>
> >> ---
> >>  core/notifier.c           |  7 ++++---
> >>  core/progress_thread.c    | 11 ++++++++---
> >>  include/progress.h        |  9 ---------
> >>  include/progress_ipc.h    |  1 +
> >>  tools/swupdate-progress.c |  4 ++--
> >>  5 files changed, 15 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/core/notifier.c b/core/notifier.c
> >> index 276e155..e4ca3c6 100644
> >> --- a/core/notifier.c
> >> +++ b/core/notifier.c
> >> @@ -339,15 +339,16 @@ static void process_notifier (RECOVERY_STATUS status, int event, int level, cons
> >>   */
> >>  static void progress_notifier (RECOVERY_STATUS status, int event, int level, const char *msg)
> >>  {
> >> +	int dwl_percent = 0;
> >> +	unsigned long long dwl_bytes = 0;
> >>  	(void)level;
> >>  
> >>  	/* Check just in case a process want to send an info outside */
> >>  	if (status != PROGRESS)
> >>  	       return;
> >>  
> >> -	if (event == RECOVERY_DWL) {
> >> -		struct progress_dwl_data *pdwl = (struct progress_dwl_data *)msg;
> >> -		swupdate_download_update(pdwl->dwl_percent, pdwl->dwl_bytes);
> >> +	if (event == RECOVERY_DWL && (sscanf(msg, "%d-%llu", &dwl_percent, &dwl_bytes) == 2)) {
> >> +		swupdate_download_update(dwl_percent, dwl_bytes);
> >>  		return;
> >>  	}
> >>  
> >> diff --git a/core/progress_thread.c b/core/progress_thread.c
> >> index cc556c1..7c424fc 100644
> >> --- a/core/progress_thread.c
> >> +++ b/core/progress_thread.c
> >> @@ -97,7 +97,7 @@ static void _swupdate_download_update(unsigned int perc, unsigned long long tota
> >>  	pthread_mutex_lock(&pprog->lock);
> >>  	if (perc != pprog->msg.dwl_percent) {
> >>  		pprog->msg.dwl_percent = perc;
> >> -		totalbytes = totalbytes;
> >> +		pprog->msg.dwl_bytes = totalbytes;
> >>  		send_progress_msg();
> >>  	}
> >>  	pthread_mutex_unlock(&pprog->lock);
> >> @@ -138,8 +138,13 @@ void swupdate_download_update(unsigned int perc, unsigned long long totalbytes)
> >>  	 * Not called by main process, for example by suricatta or Webserver
> >>  	 */
> >>  	if (pid == getpid()) {
> >> -		struct progress_dwl_data *pdwl = (struct progress_dwl_data *)info;
> >> -		pdwl->dwl_percent = perc;
> >> +		/*
> >> +		 * Notify can just receive a string as message
> >> +		 * so it is necessary to encode further information as string
> >> +		 * and decode them in the notifier, in this case
> >> +		 * the progress_notifier
> >> +		 */
> >> +		snprintf(info, sizeof(info) - 1, "%d-%llu", perc, totalbytes);
> >>  		notify(PROGRESS, RECOVERY_DWL, TRACELEVEL, info);
> >>  		return;
> >>  	}
> >> diff --git a/include/progress.h b/include/progress.h
> >> index 738e59f..9297cf9 100644
> >> --- a/include/progress.h
> >> +++ b/include/progress.h
> >> @@ -11,15 +11,6 @@
> >>  #include <swupdate_status.h>
> >>  #include <progress_ipc.h>
> >>  
> >> -/*
> >> - * Internal structures to be used to forward progress data
> >> - */
> >> -
> >> -struct progress_dwl_data {
> >> -	unsigned int	dwl_percent;	/* % downloaded data */
> >> -	unsigned long long dwl_bytes;	/* total of bytes to be downloaded */
> >> -};
> >> -
> >>  /*
> >>   * Internal SWUpdate functions to drive the progress
> >>   * interface. Common progress definitions for internal
> >> diff --git a/include/progress_ipc.h b/include/progress_ipc.h
> >> index f508eda..2efa61d 100644
> >> --- a/include/progress_ipc.h
> >> +++ b/include/progress_ipc.h
> >> @@ -27,6 +27,7 @@ struct progress_msg {
> >>  	unsigned int	magic;		/* Magic Number */
> >>  	RECOVERY_STATUS	status;		/* Update Status (Running, Failure) */
> >>  	unsigned int	dwl_percent;	/* % downloaded data */
> >> +	unsigned long long dwl_bytes;   /* total of bytes to be downloaded */
> >>  	unsigned int	nsteps;		/* No. total of steps */
> >>  	unsigned int	cur_step;	/* Current step index */
> >>  	unsigned int	cur_percent;	/* % in current step */
> >> diff --git a/tools/swupdate-progress.c b/tools/swupdate-progress.c
> >> index ecc0b40..754d56f 100644
> >> --- a/tools/swupdate-progress.c
> >> +++ b/tools/swupdate-progress.c
> >> @@ -327,11 +327,11 @@ int main(int argc, char **argv)
> >>  				fill_progress_bar(bar, sizeof(bar), msg.cur_percent);
> >>  
> >>  				if (!silent) {
> >> -					fprintf(stdout, "[ %.*s ] %d of %d %d%% (%s) dwl %d%%\r",
> >> +					fprintf(stdout, "[ %.*s ] %d of %d %d%% (%s), dwl %d%% of %llu bytes\r",
> >>  						bar_len,
> >>  						bar,
> >>  						msg.cur_step, msg.nsteps, msg.cur_percent,
> >> -						msg.cur_image, msg.dwl_percent);
> >> +						msg.cur_image, msg.dwl_percent, msg.dwl_bytes);
> >>  					fflush(stdout);
> >>  				}
> >>  
> >> -- 
> >> 2.25.1
> >>
> > 
> > Just found that this breaks core/syslog.c by it emitting messages like
> >   FATAL_UNKNOWN 15-217716224
> > with the message being the <percent>-<filesize> encoding.
> > 
> > The 'FATAL_' prefix stems from this line in core/syslog.c
> >   syslog(logprio, "%s%s %s\n", ((error != (int)RECOVERY_NO_ERROR) ? "FATAL_" : ""), statusMsg, msg);
> > where error == RECOVERY_DWL should also not result in the 'FATAL_' prefix.
> > 
> > Then, 
> >   switch(status) {
> > should also cover 
> >   case PROGRESS: statusMsg = "PROGRESS"; break;
> > and probably also SUBPROCESS for completeness.
> > 
> > This silences the FATAL alarm but still you get logged the encoded
> > information in <percent>-<filesize> syntax. IMO, it should print
> > something more friendly to human interpretation :)
> 
> Well, but this generates an overflow of dummy messages for syslog
> without meaning, as the message is addressed to the progress interface
> instead of the logging. I will tend to not log unknown sources instead
> ofadding them. So like:
> 
>       case DONE: statusMsg = "DONE"; break;
>       default:
> 	return;

Yes, suppressing those progress messages would also be an option,
agreed.


Kind regards,
   Christian
Stefano Babic Nov. 18, 2020, 5:06 p.m. UTC | #4
On 18.11.20 18:04, Christian Storm wrote:
> Hi Stefano,
> 
>>>> commit 5738732 disables the number of downloaded bytes because buggy,
>>>> this fix the behavior and ensures that the value is correctly forwareded
>>>> to the listeners.
>>>>
>>>> Signed-off-by: Stefano Babic <sbabic@denx.de>
>>>> ---
>>>>  core/notifier.c           |  7 ++++---
>>>>  core/progress_thread.c    | 11 ++++++++---
>>>>  include/progress.h        |  9 ---------
>>>>  include/progress_ipc.h    |  1 +
>>>>  tools/swupdate-progress.c |  4 ++--
>>>>  5 files changed, 15 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/core/notifier.c b/core/notifier.c
>>>> index 276e155..e4ca3c6 100644
>>>> --- a/core/notifier.c
>>>> +++ b/core/notifier.c
>>>> @@ -339,15 +339,16 @@ static void process_notifier (RECOVERY_STATUS status, int event, int level, cons
>>>>   */
>>>>  static void progress_notifier (RECOVERY_STATUS status, int event, int level, const char *msg)
>>>>  {
>>>> +	int dwl_percent = 0;
>>>> +	unsigned long long dwl_bytes = 0;
>>>>  	(void)level;
>>>>  
>>>>  	/* Check just in case a process want to send an info outside */
>>>>  	if (status != PROGRESS)
>>>>  	       return;
>>>>  
>>>> -	if (event == RECOVERY_DWL) {
>>>> -		struct progress_dwl_data *pdwl = (struct progress_dwl_data *)msg;
>>>> -		swupdate_download_update(pdwl->dwl_percent, pdwl->dwl_bytes);
>>>> +	if (event == RECOVERY_DWL && (sscanf(msg, "%d-%llu", &dwl_percent, &dwl_bytes) == 2)) {
>>>> +		swupdate_download_update(dwl_percent, dwl_bytes);
>>>>  		return;
>>>>  	}
>>>>  
>>>> diff --git a/core/progress_thread.c b/core/progress_thread.c
>>>> index cc556c1..7c424fc 100644
>>>> --- a/core/progress_thread.c
>>>> +++ b/core/progress_thread.c
>>>> @@ -97,7 +97,7 @@ static void _swupdate_download_update(unsigned int perc, unsigned long long tota
>>>>  	pthread_mutex_lock(&pprog->lock);
>>>>  	if (perc != pprog->msg.dwl_percent) {
>>>>  		pprog->msg.dwl_percent = perc;
>>>> -		totalbytes = totalbytes;
>>>> +		pprog->msg.dwl_bytes = totalbytes;
>>>>  		send_progress_msg();
>>>>  	}
>>>>  	pthread_mutex_unlock(&pprog->lock);
>>>> @@ -138,8 +138,13 @@ void swupdate_download_update(unsigned int perc, unsigned long long totalbytes)
>>>>  	 * Not called by main process, for example by suricatta or Webserver
>>>>  	 */
>>>>  	if (pid == getpid()) {
>>>> -		struct progress_dwl_data *pdwl = (struct progress_dwl_data *)info;
>>>> -		pdwl->dwl_percent = perc;
>>>> +		/*
>>>> +		 * Notify can just receive a string as message
>>>> +		 * so it is necessary to encode further information as string
>>>> +		 * and decode them in the notifier, in this case
>>>> +		 * the progress_notifier
>>>> +		 */
>>>> +		snprintf(info, sizeof(info) - 1, "%d-%llu", perc, totalbytes);
>>>>  		notify(PROGRESS, RECOVERY_DWL, TRACELEVEL, info);
>>>>  		return;
>>>>  	}
>>>> diff --git a/include/progress.h b/include/progress.h
>>>> index 738e59f..9297cf9 100644
>>>> --- a/include/progress.h
>>>> +++ b/include/progress.h
>>>> @@ -11,15 +11,6 @@
>>>>  #include <swupdate_status.h>
>>>>  #include <progress_ipc.h>
>>>>  
>>>> -/*
>>>> - * Internal structures to be used to forward progress data
>>>> - */
>>>> -
>>>> -struct progress_dwl_data {
>>>> -	unsigned int	dwl_percent;	/* % downloaded data */
>>>> -	unsigned long long dwl_bytes;	/* total of bytes to be downloaded */
>>>> -};
>>>> -
>>>>  /*
>>>>   * Internal SWUpdate functions to drive the progress
>>>>   * interface. Common progress definitions for internal
>>>> diff --git a/include/progress_ipc.h b/include/progress_ipc.h
>>>> index f508eda..2efa61d 100644
>>>> --- a/include/progress_ipc.h
>>>> +++ b/include/progress_ipc.h
>>>> @@ -27,6 +27,7 @@ struct progress_msg {
>>>>  	unsigned int	magic;		/* Magic Number */
>>>>  	RECOVERY_STATUS	status;		/* Update Status (Running, Failure) */
>>>>  	unsigned int	dwl_percent;	/* % downloaded data */
>>>> +	unsigned long long dwl_bytes;   /* total of bytes to be downloaded */
>>>>  	unsigned int	nsteps;		/* No. total of steps */
>>>>  	unsigned int	cur_step;	/* Current step index */
>>>>  	unsigned int	cur_percent;	/* % in current step */
>>>> diff --git a/tools/swupdate-progress.c b/tools/swupdate-progress.c
>>>> index ecc0b40..754d56f 100644
>>>> --- a/tools/swupdate-progress.c
>>>> +++ b/tools/swupdate-progress.c
>>>> @@ -327,11 +327,11 @@ int main(int argc, char **argv)
>>>>  				fill_progress_bar(bar, sizeof(bar), msg.cur_percent);
>>>>  
>>>>  				if (!silent) {
>>>> -					fprintf(stdout, "[ %.*s ] %d of %d %d%% (%s) dwl %d%%\r",
>>>> +					fprintf(stdout, "[ %.*s ] %d of %d %d%% (%s), dwl %d%% of %llu bytes\r",
>>>>  						bar_len,
>>>>  						bar,
>>>>  						msg.cur_step, msg.nsteps, msg.cur_percent,
>>>> -						msg.cur_image, msg.dwl_percent);
>>>> +						msg.cur_image, msg.dwl_percent, msg.dwl_bytes);
>>>>  					fflush(stdout);
>>>>  				}
>>>>  
>>>> -- 
>>>> 2.25.1
>>>>
>>>
>>> Just found that this breaks core/syslog.c by it emitting messages like
>>>   FATAL_UNKNOWN 15-217716224
>>> with the message being the <percent>-<filesize> encoding.
>>>
>>> The 'FATAL_' prefix stems from this line in core/syslog.c
>>>   syslog(logprio, "%s%s %s\n", ((error != (int)RECOVERY_NO_ERROR) ? "FATAL_" : ""), statusMsg, msg);
>>> where error == RECOVERY_DWL should also not result in the 'FATAL_' prefix.
>>>
>>> Then, 
>>>   switch(status) {
>>> should also cover 
>>>   case PROGRESS: statusMsg = "PROGRESS"; break;
>>> and probably also SUBPROCESS for completeness.
>>>
>>> This silences the FATAL alarm but still you get logged the encoded
>>> information in <percent>-<filesize> syntax. IMO, it should print
>>> something more friendly to human interpretation :)
>>
>> Well, but this generates an overflow of dummy messages for syslog
>> without meaning, as the message is addressed to the progress interface
>> instead of the logging. I will tend to not log unknown sources instead
>> ofadding them. So like:
>>
>>       case DONE: statusMsg = "DONE"; break;
>>       default:
>> 	return;
> 
> Yes, suppressing those progress messages would also be an option,
> agreed.
> 

I send a patch...

Stefano

> 
> Kind regards,
>    Christian
>
diff mbox series

Patch

diff --git a/core/notifier.c b/core/notifier.c
index 276e155..e4ca3c6 100644
--- a/core/notifier.c
+++ b/core/notifier.c
@@ -339,15 +339,16 @@  static void process_notifier (RECOVERY_STATUS status, int event, int level, cons
  */
 static void progress_notifier (RECOVERY_STATUS status, int event, int level, const char *msg)
 {
+	int dwl_percent = 0;
+	unsigned long long dwl_bytes = 0;
 	(void)level;
 
 	/* Check just in case a process want to send an info outside */
 	if (status != PROGRESS)
 	       return;
 
-	if (event == RECOVERY_DWL) {
-		struct progress_dwl_data *pdwl = (struct progress_dwl_data *)msg;
-		swupdate_download_update(pdwl->dwl_percent, pdwl->dwl_bytes);
+	if (event == RECOVERY_DWL && (sscanf(msg, "%d-%llu", &dwl_percent, &dwl_bytes) == 2)) {
+		swupdate_download_update(dwl_percent, dwl_bytes);
 		return;
 	}
 
diff --git a/core/progress_thread.c b/core/progress_thread.c
index cc556c1..7c424fc 100644
--- a/core/progress_thread.c
+++ b/core/progress_thread.c
@@ -97,7 +97,7 @@  static void _swupdate_download_update(unsigned int perc, unsigned long long tota
 	pthread_mutex_lock(&pprog->lock);
 	if (perc != pprog->msg.dwl_percent) {
 		pprog->msg.dwl_percent = perc;
-		totalbytes = totalbytes;
+		pprog->msg.dwl_bytes = totalbytes;
 		send_progress_msg();
 	}
 	pthread_mutex_unlock(&pprog->lock);
@@ -138,8 +138,13 @@  void swupdate_download_update(unsigned int perc, unsigned long long totalbytes)
 	 * Not called by main process, for example by suricatta or Webserver
 	 */
 	if (pid == getpid()) {
-		struct progress_dwl_data *pdwl = (struct progress_dwl_data *)info;
-		pdwl->dwl_percent = perc;
+		/*
+		 * Notify can just receive a string as message
+		 * so it is necessary to encode further information as string
+		 * and decode them in the notifier, in this case
+		 * the progress_notifier
+		 */
+		snprintf(info, sizeof(info) - 1, "%d-%llu", perc, totalbytes);
 		notify(PROGRESS, RECOVERY_DWL, TRACELEVEL, info);
 		return;
 	}
diff --git a/include/progress.h b/include/progress.h
index 738e59f..9297cf9 100644
--- a/include/progress.h
+++ b/include/progress.h
@@ -11,15 +11,6 @@ 
 #include <swupdate_status.h>
 #include <progress_ipc.h>
 
-/*
- * Internal structures to be used to forward progress data
- */
-
-struct progress_dwl_data {
-	unsigned int	dwl_percent;	/* % downloaded data */
-	unsigned long long dwl_bytes;	/* total of bytes to be downloaded */
-};
-
 /*
  * Internal SWUpdate functions to drive the progress
  * interface. Common progress definitions for internal
diff --git a/include/progress_ipc.h b/include/progress_ipc.h
index f508eda..2efa61d 100644
--- a/include/progress_ipc.h
+++ b/include/progress_ipc.h
@@ -27,6 +27,7 @@  struct progress_msg {
 	unsigned int	magic;		/* Magic Number */
 	RECOVERY_STATUS	status;		/* Update Status (Running, Failure) */
 	unsigned int	dwl_percent;	/* % downloaded data */
+	unsigned long long dwl_bytes;   /* total of bytes to be downloaded */
 	unsigned int	nsteps;		/* No. total of steps */
 	unsigned int	cur_step;	/* Current step index */
 	unsigned int	cur_percent;	/* % in current step */
diff --git a/tools/swupdate-progress.c b/tools/swupdate-progress.c
index ecc0b40..754d56f 100644
--- a/tools/swupdate-progress.c
+++ b/tools/swupdate-progress.c
@@ -327,11 +327,11 @@  int main(int argc, char **argv)
 				fill_progress_bar(bar, sizeof(bar), msg.cur_percent);
 
 				if (!silent) {
-					fprintf(stdout, "[ %.*s ] %d of %d %d%% (%s) dwl %d%%\r",
+					fprintf(stdout, "[ %.*s ] %d of %d %d%% (%s), dwl %d%% of %llu bytes\r",
 						bar_len,
 						bar,
 						msg.cur_step, msg.nsteps, msg.cur_percent,
-						msg.cur_image, msg.dwl_percent);
+						msg.cur_image, msg.dwl_percent, msg.dwl_bytes);
 					fflush(stdout);
 				}