diff mbox series

[2/6] swupdate exit code: return exit status from main child process on main process

Message ID 20220422235944.2808227-2-dominique.martinet@atmark-techno.com
State Accepted
Headers show
Series [1/6] sigchld_handler: report child exit status correctly | expand

Commit Message

Dominique MARTINET April 22, 2022, 11:59 p.m. UTC 
child processes such as start_download take care of returning a valid exit
code, but that code is lost right now.
Instead it's possible to just have swupdate forward it out of the main
process (currently exiting with success all the time),
so e.g. swupdate -d with a failing update fails appropriately

Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
---

Note this change exit status for e.g. swupdate -d, so might not be
compatible for some existing scripts that expect swupdate to return 0
when update failed with these.
These scripts have however no way of knowing if an update failed without
parsing swupdate output, so I think this is better, but I'm leaving the
decision here to you.
I don't actually rely on the exit code anywhere for this (although I did
think of using it without knowing it wouldn't work...)


 core/pctl.c     |  4 +++-
 core/swupdate.c | 10 +++++-----
 include/util.h  |  1 +
 3 files changed, 9 insertions(+), 6 deletions(-)

Comments

Stefano Babic May 6, 2022, 7:01 a.m. UTC | #1 
On 23.04.22 01:59, Dominique Martinet wrote:
> child processes such as start_download take care of returning a valid exit
> code, but that code is lost right now.
> Instead it's possible to just have swupdate forward it out of the main
> process (currently exiting with success all the time),
> so e.g. swupdate -d with a failing update fails appropriately
> 
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> ---
> 
> Note this change exit status for e.g. swupdate -d, so might not be
> compatible for some existing scripts that expect swupdate to return 0
> when update failed with these.
> These scripts have however no way of knowing if an update failed without
> parsing swupdate output, so I think this is better, but I'm leaving the
> decision here to you.
> I don't actually rely on the exit code anywhere for this (although I did
> think of using it without knowing it wouldn't work...)
> 
> 
>   core/pctl.c     |  4 +++-
>   core/swupdate.c | 10 +++++-----
>   include/util.h  |  1 +
>   3 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/core/pctl.c b/core/pctl.c
> index ed5d49bbbf01..1c6e556d8721 100644
> --- a/core/pctl.c
> +++ b/core/pctl.c
> @@ -429,9 +429,11 @@ void sigchld_handler (int __attribute__ ((__unused__)) signum)
>   			hasdied = 0;
>   			if (WIFEXITED(status)) {
>   				hasdied = 1;
> -				printf("exited, status=%d\n", WEXITSTATUS(status));
> +				exit_code = WEXITSTATUS(status);
> +				printf("exited, status=%d\n", exit_code);
>   			} else if (WIFSIGNALED(status)) {
>   				hasdied = 1;
> +				exit_code = EXIT_FAILURE;
>   				printf("killed by signal %d\n", WTERMSIG(status));
>   			} else if (WIFSTOPPED(status)) {
>   				printf("stopped by signal %d\n", WSTOPSIG(status));
> diff --git a/core/swupdate.c b/core/swupdate.c
> index f06497fbebbd..3c9c8cb6a100 100644
> --- a/core/swupdate.c
> +++ b/core/swupdate.c
> @@ -60,6 +60,9 @@ static pthread_t network_daemon;
>   /* Tree derived from the configuration file */
>   static struct swupdate_cfg swcfg;
>   
> +int loglevel = ERRORLEVEL;
> +int exit_code = EXIT_SUCCESS;
> +
>   #ifdef CONFIG_MTD
>   /* Global MTD configuration */
>   static struct flash_description flashdesc;
> @@ -118,8 +121,6 @@ static struct option long_options[] = {
>   	{NULL, 0, NULL, 0}
>   };
>   
> -int loglevel = ERRORLEVEL;
> -
>   static void usage(char *programname)
>   {
>   	fprintf(stdout, "%s (compiled %s)\n", programname, __DATE__);
> @@ -421,7 +422,6 @@ int main(int argc, char **argv)
>   	char main_options[256];
>   	unsigned int public_key_mandatory = 0;
>   	struct sigaction sa;
> -	int result = EXIT_SUCCESS;
>   #ifdef CONFIG_SURICATTA
>   	int opt_u = 0;
>   	char *suricattaoptions;
> @@ -919,7 +919,7 @@ int main(int argc, char **argv)
>   	}
>   
>   	if (opt_i) {
> -		result = install_from_file(fname, opt_c);
> +		exit_code = install_from_file(fname, opt_c);
>   	}
>   
>   #ifdef CONFIG_SYSTEMD
> @@ -945,5 +945,5 @@ int main(int argc, char **argv)
>   	if (!opt_c && !opt_i)
>   		pthread_join(network_daemon, NULL);
>   
> -	return result;
> +	return exit_code;
>   }
> diff --git a/include/util.h b/include/util.h
> index 302ea04c7ba2..6a3295ce45a9 100644
> --- a/include/util.h
> +++ b/include/util.h
> @@ -33,6 +33,7 @@
>   #define SWUPDATE_ALIGN(A,S)    (((A) + (S) - 1) & ~((S) - 1))
>   
>   extern int loglevel;
> +extern int exit_code;
>   
>   typedef enum {
>   	SERVER_OK,

Thanks for this !

Reviewed-by: Stefano babic <sbabic@denx.de>

Best regards,
Stefano Babic
Stefano Babic May 10, 2022, 10:37 a.m. UTC | #2 
On 23.04.22 01:59, Dominique Martinet wrote:
> child processes such as start_download take care of returning a valid exit
> code, but that code is lost right now.
> Instead it's possible to just have swupdate forward it out of the main
> process (currently exiting with success all the time),
> so e.g. swupdate -d with a failing update fails appropriately
> 
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> ---
> 
> Note this change exit status for e.g. swupdate -d, so might not be
> compatible for some existing scripts that expect swupdate to return 0
> when update failed with these.
> These scripts have however no way of knowing if an update failed without
> parsing swupdate output, so I think this is better, but I'm leaving the
> decision here to you.
> I don't actually rely on the exit code anywhere for this (although I did
> think of using it without knowing it wouldn't work...)
> 
> 
>   core/pctl.c     |  4 +++-
>   core/swupdate.c | 10 +++++-----
>   include/util.h  |  1 +
>   3 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/core/pctl.c b/core/pctl.c
> index ed5d49bbbf01..1c6e556d8721 100644
> --- a/core/pctl.c
> +++ b/core/pctl.c
> @@ -429,9 +429,11 @@ void sigchld_handler (int __attribute__ ((__unused__)) signum)
>   			hasdied = 0;
>   			if (WIFEXITED(status)) {
>   				hasdied = 1;
> -				printf("exited, status=%d\n", WEXITSTATUS(status));
> +				exit_code = WEXITSTATUS(status);
> +				printf("exited, status=%d\n", exit_code);
>   			} else if (WIFSIGNALED(status)) {
>   				hasdied = 1;
> +				exit_code = EXIT_FAILURE;
>   				printf("killed by signal %d\n", WTERMSIG(status));
>   			} else if (WIFSTOPPED(status)) {
>   				printf("stopped by signal %d\n", WSTOPSIG(status));
> diff --git a/core/swupdate.c b/core/swupdate.c
> index f06497fbebbd..3c9c8cb6a100 100644
> --- a/core/swupdate.c
> +++ b/core/swupdate.c
> @@ -60,6 +60,9 @@ static pthread_t network_daemon;
>   /* Tree derived from the configuration file */
>   static struct swupdate_cfg swcfg;
>   
> +int loglevel = ERRORLEVEL;
> +int exit_code = EXIT_SUCCESS;
> +
>   #ifdef CONFIG_MTD
>   /* Global MTD configuration */
>   static struct flash_description flashdesc;
> @@ -118,8 +121,6 @@ static struct option long_options[] = {
>   	{NULL, 0, NULL, 0}
>   };
>   
> -int loglevel = ERRORLEVEL;
> -
>   static void usage(char *programname)
>   {
>   	fprintf(stdout, "%s (compiled %s)\n", programname, __DATE__);
> @@ -421,7 +422,6 @@ int main(int argc, char **argv)
>   	char main_options[256];
>   	unsigned int public_key_mandatory = 0;
>   	struct sigaction sa;
> -	int result = EXIT_SUCCESS;
>   #ifdef CONFIG_SURICATTA
>   	int opt_u = 0;
>   	char *suricattaoptions;
> @@ -919,7 +919,7 @@ int main(int argc, char **argv)
>   	}
>   
>   	if (opt_i) {
> -		result = install_from_file(fname, opt_c);
> +		exit_code = install_from_file(fname, opt_c);
>   	}
>   
>   #ifdef CONFIG_SYSTEMD
> @@ -945,5 +945,5 @@ int main(int argc, char **argv)
>   	if (!opt_c && !opt_i)
>   		pthread_join(network_daemon, NULL);
>   
> -	return result;
> +	return exit_code;
>   }
> diff --git a/include/util.h b/include/util.h
> index 302ea04c7ba2..6a3295ce45a9 100644
> --- a/include/util.h
> +++ b/include/util.h
> @@ -33,6 +33,7 @@
>   #define SWUPDATE_ALIGN(A,S)    (((A) + (S) - 1) & ~((S) - 1))
>   
>   extern int loglevel;
> +extern int exit_code;
>   
>   typedef enum {
>   	SERVER_OK,

Applied to -master, thanks !

Best regards,
Stefano Babic
diff mbox series

Patch

diff --git a/core/pctl.c b/core/pctl.c
index ed5d49bbbf01..1c6e556d8721 100644
--- a/core/pctl.c
+++ b/core/pctl.c
@@ -429,9 +429,11 @@  void sigchld_handler (int __attribute__ ((__unused__)) signum)
 			hasdied = 0;
 			if (WIFEXITED(status)) {
 				hasdied = 1;
-				printf("exited, status=%d\n", WEXITSTATUS(status));
+				exit_code = WEXITSTATUS(status);
+				printf("exited, status=%d\n", exit_code);
 			} else if (WIFSIGNALED(status)) {
 				hasdied = 1;
+				exit_code = EXIT_FAILURE;
 				printf("killed by signal %d\n", WTERMSIG(status));
 			} else if (WIFSTOPPED(status)) {
 				printf("stopped by signal %d\n", WSTOPSIG(status));
diff --git a/core/swupdate.c b/core/swupdate.c
index f06497fbebbd..3c9c8cb6a100 100644
--- a/core/swupdate.c
+++ b/core/swupdate.c
@@ -60,6 +60,9 @@  static pthread_t network_daemon;
 /* Tree derived from the configuration file */
 static struct swupdate_cfg swcfg;
 
+int loglevel = ERRORLEVEL;
+int exit_code = EXIT_SUCCESS;
+
 #ifdef CONFIG_MTD
 /* Global MTD configuration */
 static struct flash_description flashdesc;
@@ -118,8 +121,6 @@  static struct option long_options[] = {
 	{NULL, 0, NULL, 0}
 };
 
-int loglevel = ERRORLEVEL;
-
 static void usage(char *programname)
 {
 	fprintf(stdout, "%s (compiled %s)\n", programname, __DATE__);
@@ -421,7 +422,6 @@  int main(int argc, char **argv)
 	char main_options[256];
 	unsigned int public_key_mandatory = 0;
 	struct sigaction sa;
-	int result = EXIT_SUCCESS;
 #ifdef CONFIG_SURICATTA
 	int opt_u = 0;
 	char *suricattaoptions;
@@ -919,7 +919,7 @@  int main(int argc, char **argv)
 	}
 
 	if (opt_i) {
-		result = install_from_file(fname, opt_c);
+		exit_code = install_from_file(fname, opt_c);
 	}
 
 #ifdef CONFIG_SYSTEMD
@@ -945,5 +945,5 @@  int main(int argc, char **argv)
 	if (!opt_c && !opt_i)
 		pthread_join(network_daemon, NULL);
 
-	return result;
+	return exit_code;
 }
diff --git a/include/util.h b/include/util.h
index 302ea04c7ba2..6a3295ce45a9 100644
--- a/include/util.h
+++ b/include/util.h
@@ -33,6 +33,7 @@ 
 #define SWUPDATE_ALIGN(A,S)    (((A) + (S) - 1) & ~((S) - 1))
 
 extern int loglevel;
+extern int exit_code;
 
 typedef enum {
 	SERVER_OK,