| 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 |
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
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 --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 {
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(+)