diff mbox series

[2/5] Rework emit download progress

Message ID 20201024184730.119506-2-sbabic@denx.de
State Changes Requested
Headers show
Series [1/5] downloader: drop unuseful marker when dwl is started | expand

Commit Message

Stefano Babic Oct. 24, 2020, 6:47 p.m. UTC
There is a notification to signal how many bytes are downloaded, but
this was later introduced and does not match SWUpdate's design. In fact,
because it is not called by the main process, it uses the notifier to
send messages to the main process. The notification caused in the past
flood of data to the installer, and does not fill the foreseen fields in
the progress IPC to forward download statistics.

This adds a swupdate_download_update() that can be called by any
process. If it is called by the main process, emit a progress message to
the listeners, else it prepares a message and sends it as notification
to the progress notifier. A new enum for the status (RECOVERY_DWL) is
added to let the progress notifier to identify the type of message.

Signed-off-by: Stefano Babic <sbabic@denx.de>
---
 core/notifier.c        |  6 ++++++
 core/progress_thread.c | 32 ++++++++++++++++++++++++++++++++
 include/progress.h     | 11 +++++++++++
 include/progress_ipc.h |  1 +
 include/util.h         |  1 +
 5 files changed, 51 insertions(+)

Comments

Christian Storm Oct. 26, 2020, 8:14 a.m. UTC | #1
Hi Stefano,

> There is a notification to signal how many bytes are downloaded, but
> this was later introduced and does not match SWUpdate's design. In fact,
> because it is not called by the main process, it uses the notifier to
> send messages to the main process. The notification caused in the past
> flood of data to the installer, and does not fill the foreseen fields in
> the progress IPC to forward download statistics.
> 
> This adds a swupdate_download_update() that can be called by any
> process. If it is called by the main process, emit a progress message to
> the listeners, else it prepares a message and sends it as notification
> to the progress notifier. A new enum for the status (RECOVERY_DWL) is
> added to let the progress notifier to identify the type of message.
> 
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> ---
>  core/notifier.c        |  6 ++++++
>  core/progress_thread.c | 32 ++++++++++++++++++++++++++++++++
>  include/progress.h     | 11 +++++++++++
>  include/progress_ipc.h |  1 +
>  include/util.h         |  1 +
>  5 files changed, 51 insertions(+)
> 
> diff --git a/core/notifier.c b/core/notifier.c
> index 3b8a432..276e155 100644
> --- a/core/notifier.c
> +++ b/core/notifier.c
> @@ -345,6 +345,12 @@ static void progress_notifier (RECOVERY_STATUS status, int event, int level, con
>  	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);
> +		return;
> +	}
> +
>  	swupdate_progress_info(status, event, msg);
>  }
>  
> diff --git a/core/progress_thread.c b/core/progress_thread.c
> index 157498a..1889d72 100644
> --- a/core/progress_thread.c
> +++ b/core/progress_thread.c
> @@ -25,6 +25,7 @@
>  #include "swupdate.h"
>  #include <handler.h>
>  #include "util.h"
> +#include "pctl.h"
>  #include "network_ipc.h"
>  #include "network_interface.h"
>  #include <progress.h>
> @@ -86,6 +87,18 @@ static void send_progress_msg(void)
>  	}
>  }
>  
> +static void _swupdate_download_update(unsigned int perc, unsigned long long totalbytes)
> +{
> +	struct swupdate_progress *prbar = &progress;

Hm, I think the name prbar is misleading and long superseded as the
progress interface is also used for different purposes than displaying
a progress bar. So, maybe we can invent a new and more telling name?


> +	pthread_mutex_lock(&prbar->lock);
> +	if (perc != prbar->msg.dwl_percent) {
> +		prbar->msg.dwl_percent = perc;
> +		prbar->msg.dwl_bytes = totalbytes;
> +		send_progress_msg();
> +	}
> +	pthread_mutex_unlock(&prbar->lock);
> +}
> +
>  void swupdate_progress_init(unsigned int nsteps) {
>  	struct swupdate_progress *prbar = &progress;
>  	pthread_mutex_lock(&prbar->lock);
> @@ -113,6 +126,25 @@ void swupdate_progress_update(unsigned int perc)
>  	pthread_mutex_unlock(&prbar->lock);
>  }
>  
> +void swupdate_download_update(unsigned int perc, unsigned long long totalbytes)
> +{
> +	char	info[2048];   		/* info */


Hm, this 2048 is hard-coded all over the place in SWUpdate (for various
usages), maybe we can make some #define's for it so to better manage
consistency?


> +
> +	/*
> +	 * 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;
> +		pdwl->dwl_bytes = totalbytes;
> +		notify(PROGRESS, RECOVERY_DWL, TRACELEVEL, info);
> +		return;
> +	}
> +
> +	/* Called by main process, emit a progress message */
> +	_swupdate_download_update(perc, totalbytes);
> +}
> +
>  void swupdate_progress_inc_step(char *image, char *handler_name)
>  {
>  	struct swupdate_progress *prbar = &progress;
> diff --git a/include/progress.h b/include/progress.h
> index b4d09d2..738e59f 100644
> --- a/include/progress.h
> +++ b/include/progress.h
> @@ -11,6 +11,15 @@
>  #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
> @@ -24,6 +33,8 @@ void swupdate_progress_end(RECOVERY_STATUS status);
>  void swupdate_progress_done(const char *info);
>  void swupdate_progress_info(RECOVERY_STATUS status, int cause, const char *msg);
>  
> +void swupdate_download_update(unsigned int perc, unsigned long long totalbytes);
> +
>  void *progress_bar_thread (void *data);
>  
>  #endif
> diff --git a/include/progress_ipc.h b/include/progress_ipc.h
> index 2c242eb..8765f23 100644
> --- a/include/progress_ipc.h
> +++ b/include/progress_ipc.h
> @@ -26,6 +26,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/include/util.h b/include/util.h
> index bf8d8a0..b6ceee6 100644
> --- a/include/util.h
> +++ b/include/util.h
> @@ -73,6 +73,7 @@ typedef enum {
>  enum {
>  	RECOVERY_NO_ERROR,
>  	RECOVERY_ERROR,
> +	RECOVERY_DWL,
>  };
>  
>  struct installer {
> -- 
> 2.25.1
> 


Kind regards,
   Christian
Stefano Babic Oct. 26, 2020, 8:42 a.m. UTC | #2
Hi Christian,

On 26.10.20 09:14, Christian Storm wrote:
> Hi Stefano,
> 
>> There is a notification to signal how many bytes are downloaded, but
>> this was later introduced and does not match SWUpdate's design. In fact,
>> because it is not called by the main process, it uses the notifier to
>> send messages to the main process. The notification caused in the past
>> flood of data to the installer, and does not fill the foreseen fields in
>> the progress IPC to forward download statistics.
>>
>> This adds a swupdate_download_update() that can be called by any
>> process. If it is called by the main process, emit a progress message to
>> the listeners, else it prepares a message and sends it as notification
>> to the progress notifier. A new enum for the status (RECOVERY_DWL) is
>> added to let the progress notifier to identify the type of message.
>>
>> Signed-off-by: Stefano Babic <sbabic@denx.de>
>> ---
>>  core/notifier.c        |  6 ++++++
>>  core/progress_thread.c | 32 ++++++++++++++++++++++++++++++++
>>  include/progress.h     | 11 +++++++++++
>>  include/progress_ipc.h |  1 +
>>  include/util.h         |  1 +
>>  5 files changed, 51 insertions(+)
>>
>> diff --git a/core/notifier.c b/core/notifier.c
>> index 3b8a432..276e155 100644
>> --- a/core/notifier.c
>> +++ b/core/notifier.c
>> @@ -345,6 +345,12 @@ static void progress_notifier (RECOVERY_STATUS status, int event, int level, con
>>  	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);
>> +		return;
>> +	}
>> +
>>  	swupdate_progress_info(status, event, msg);
>>  }
>>  
>> diff --git a/core/progress_thread.c b/core/progress_thread.c
>> index 157498a..1889d72 100644
>> --- a/core/progress_thread.c
>> +++ b/core/progress_thread.c
>> @@ -25,6 +25,7 @@
>>  #include "swupdate.h"
>>  #include <handler.h>
>>  #include "util.h"
>> +#include "pctl.h"
>>  #include "network_ipc.h"
>>  #include "network_interface.h"
>>  #include <progress.h>
>> @@ -86,6 +87,18 @@ static void send_progress_msg(void)
>>  	}
>>  }
>>  
>> +static void _swupdate_download_update(unsigned int perc, unsigned long long totalbytes)
>> +{
>> +	struct swupdate_progress *prbar = &progress;
> 
> Hm, I think the name prbar is misleading and long superseded as the
> progress interface is also used for different purposes than displaying
> a progress bar. So, maybe we can invent a new and more telling name?

I will do. Anyway, the same variable name is used in the whole file, and
"prbar" is then consistent with the rest of code. I will change then in
the whole file.

> 
> 
>> +	pthread_mutex_lock(&prbar->lock);
>> +	if (perc != prbar->msg.dwl_percent) {
>> +		prbar->msg.dwl_percent = perc;
>> +		prbar->msg.dwl_bytes = totalbytes;
>> +		send_progress_msg();
>> +	}
>> +	pthread_mutex_unlock(&prbar->lock);
>> +}
>> +
>>  void swupdate_progress_init(unsigned int nsteps) {
>>  	struct swupdate_progress *prbar = &progress;
>>  	pthread_mutex_lock(&prbar->lock);
>> @@ -113,6 +126,25 @@ void swupdate_progress_update(unsigned int perc)
>>  	pthread_mutex_unlock(&prbar->lock);
>>  }
>>  
>> +void swupdate_download_update(unsigned int perc, unsigned long long totalbytes)
>> +{
>> +	char	info[2048];   		/* info */
> 
> 
> Hm, this 2048 is hard-coded all over the place in SWUpdate (for various
> usages), maybe we can make some #define's for it so to better manage
> consistency?

I'll do it.

> 
> 
>> +
>> +	/*
>> +	 * 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;
>> +		pdwl->dwl_bytes = totalbytes;
>> +		notify(PROGRESS, RECOVERY_DWL, TRACELEVEL, info);
>> +		return;
>> +	}
>> +
>> +	/* Called by main process, emit a progress message */
>> +	_swupdate_download_update(perc, totalbytes);
>> +}
>> +
>>  void swupdate_progress_inc_step(char *image, char *handler_name)
>>  {
>>  	struct swupdate_progress *prbar = &progress;
>> diff --git a/include/progress.h b/include/progress.h
>> index b4d09d2..738e59f 100644
>> --- a/include/progress.h
>> +++ b/include/progress.h
>> @@ -11,6 +11,15 @@
>>  #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
>> @@ -24,6 +33,8 @@ void swupdate_progress_end(RECOVERY_STATUS status);
>>  void swupdate_progress_done(const char *info);
>>  void swupdate_progress_info(RECOVERY_STATUS status, int cause, const char *msg);
>>  
>> +void swupdate_download_update(unsigned int perc, unsigned long long totalbytes);
>> +
>>  void *progress_bar_thread (void *data);
>>  
>>  #endif
>> diff --git a/include/progress_ipc.h b/include/progress_ipc.h
>> index 2c242eb..8765f23 100644
>> --- a/include/progress_ipc.h
>> +++ b/include/progress_ipc.h
>> @@ -26,6 +26,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/include/util.h b/include/util.h
>> index bf8d8a0..b6ceee6 100644
>> --- a/include/util.h
>> +++ b/include/util.h
>> @@ -73,6 +73,7 @@ typedef enum {
>>  enum {
>>  	RECOVERY_NO_ERROR,
>>  	RECOVERY_ERROR,
>> +	RECOVERY_DWL,
>>  };
>>  
>>  struct installer {
>> -- 
>> 2.25.1
>>
> 

Best regards,
Stefano Babic
diff mbox series

Patch

diff --git a/core/notifier.c b/core/notifier.c
index 3b8a432..276e155 100644
--- a/core/notifier.c
+++ b/core/notifier.c
@@ -345,6 +345,12 @@  static void progress_notifier (RECOVERY_STATUS status, int event, int level, con
 	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);
+		return;
+	}
+
 	swupdate_progress_info(status, event, msg);
 }
 
diff --git a/core/progress_thread.c b/core/progress_thread.c
index 157498a..1889d72 100644
--- a/core/progress_thread.c
+++ b/core/progress_thread.c
@@ -25,6 +25,7 @@ 
 #include "swupdate.h"
 #include <handler.h>
 #include "util.h"
+#include "pctl.h"
 #include "network_ipc.h"
 #include "network_interface.h"
 #include <progress.h>
@@ -86,6 +87,18 @@  static void send_progress_msg(void)
 	}
 }
 
+static void _swupdate_download_update(unsigned int perc, unsigned long long totalbytes)
+{
+	struct swupdate_progress *prbar = &progress;
+	pthread_mutex_lock(&prbar->lock);
+	if (perc != prbar->msg.dwl_percent) {
+		prbar->msg.dwl_percent = perc;
+		prbar->msg.dwl_bytes = totalbytes;
+		send_progress_msg();
+	}
+	pthread_mutex_unlock(&prbar->lock);
+}
+
 void swupdate_progress_init(unsigned int nsteps) {
 	struct swupdate_progress *prbar = &progress;
 	pthread_mutex_lock(&prbar->lock);
@@ -113,6 +126,25 @@  void swupdate_progress_update(unsigned int perc)
 	pthread_mutex_unlock(&prbar->lock);
 }
 
+void swupdate_download_update(unsigned int perc, unsigned long long totalbytes)
+{
+	char	info[2048];   		/* info */
+
+	/*
+	 * 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;
+		pdwl->dwl_bytes = totalbytes;
+		notify(PROGRESS, RECOVERY_DWL, TRACELEVEL, info);
+		return;
+	}
+
+	/* Called by main process, emit a progress message */
+	_swupdate_download_update(perc, totalbytes);
+}
+
 void swupdate_progress_inc_step(char *image, char *handler_name)
 {
 	struct swupdate_progress *prbar = &progress;
diff --git a/include/progress.h b/include/progress.h
index b4d09d2..738e59f 100644
--- a/include/progress.h
+++ b/include/progress.h
@@ -11,6 +11,15 @@ 
 #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
@@ -24,6 +33,8 @@  void swupdate_progress_end(RECOVERY_STATUS status);
 void swupdate_progress_done(const char *info);
 void swupdate_progress_info(RECOVERY_STATUS status, int cause, const char *msg);
 
+void swupdate_download_update(unsigned int perc, unsigned long long totalbytes);
+
 void *progress_bar_thread (void *data);
 
 #endif
diff --git a/include/progress_ipc.h b/include/progress_ipc.h
index 2c242eb..8765f23 100644
--- a/include/progress_ipc.h
+++ b/include/progress_ipc.h
@@ -26,6 +26,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/include/util.h b/include/util.h
index bf8d8a0..b6ceee6 100644
--- a/include/util.h
+++ b/include/util.h
@@ -73,6 +73,7 @@  typedef enum {
 enum {
 	RECOVERY_NO_ERROR,
 	RECOVERY_ERROR,
+	RECOVERY_DWL,
 };
 
 struct installer {