| Message ID | 20220527045948.3672022-3-dominique.martinet@atmark-techno.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | Allow scripts to write INFO and WARN messages | expand |
Hi Dominique, On 27.05.22 06:59, Dominique Martinet wrote: > subprocesses (e.g. scripts) are currently limited to ERROR and TRACE. > > When TRACE is not displayed (as it can get quite verbose), the only way to > get messages to the user is to write to stderr which will be displayed > in red as ERROR messages. > This can be confusing when we just want to notify users e.g. of upcoming > reboot. > > This commits creates a couple of additional pipes which are just handled > like stdout and stderr, except they are displayed at info and warn > log levels. > Scripts can check for SWUPDATE_INFO_FD / SWUPDATE_WARN_FD to decide if > they can output to these file descriptors. > You already know that the way is to write Lua scripts, and this scalkes well - not only Lua runs in the context of SWUpdate's process, but it is easy to extend the API with additional functions. Lua scripts can decide if they need a TRACE, ERRO, INFO via swupdate_notify. After said this (I know it does not help), look at the concrete issue. Let's start that this is what scripts generally made available. Each script works with stdin / stdout / stderr, and a better granularity is not possible. IMHO the solution with additional pipes does not scale well: next time one developer would like to have a pipe for DEBUG, or the log levels will be extended and this requires additional pipes and complexity. And scripts mus be adjusted or written with the multiple pipes in mind. I will tend for another approach: the issue is how SWUpdate can map what the scripts is writing. Let's leave stderr just for errors. We can introduce a format for stdout, and SWUpdate can interprete this (for example, the starting bytes should contain the loglevel in some way), and SWUpdate will map this output or fallback to TRACE if not fouund to be compatible with the past. It is then even easier to modify existing scripts, and the behavior scales well. Best regards, Stefano > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > core/pctl.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 54 insertions(+), 8 deletions(-) > > diff --git a/core/pctl.c b/core/pctl.c > index 89ebb452de04..34bd1eef63f3 100644 > --- a/core/pctl.c > +++ b/core/pctl.c > @@ -12,6 +12,7 @@ > #include <sys/socket.h> > #include <fcntl.h> > #include <sys/select.h> > +#include <stdlib.h> > #if defined(__linux__) > #include <sys/prctl.h> > #endif > @@ -252,6 +253,8 @@ int run_system_cmd(const char *cmd) > int ret = 0; > int stdoutpipe[2]; > int stderrpipe[2]; > + int infopipe[2]; > + int warnpipe[2]; > pid_t process_id; > int const PIPE_READ = 0; > int const PIPE_WRITE = 1; > @@ -280,6 +283,23 @@ int run_system_cmd(const char *cmd) > close(stdoutpipe[1]); > return -EFAULT; > } > + if (pipe(infopipe) < 0) { > + close(stderrpipe[0]); > + close(stderrpipe[1]); > + close(stdoutpipe[0]); > + close(stdoutpipe[1]); > + return -EFAULT; > + } > + if (pipe(warnpipe) < 0) { > + close(infopipe[0]); > + close(infopipe[1]); > + close(stderrpipe[0]); > + close(stderrpipe[1]); > + close(stdoutpipe[0]); > + close(stdoutpipe[1]); > + return -EFAULT; > + } > + > > process_id = fork(); > /* Child process, this runs the shell command */ > @@ -288,11 +308,24 @@ int run_system_cmd(const char *cmd) > exit(errno); > if (dup2(stderrpipe[PIPE_WRITE], STDERR_FILENO) < 0) > exit(errno); > + /* posix sh cannot use fd >= 10, so dup these to lower > + * numbers for convenience */ > + if (dup2(infopipe[PIPE_WRITE], 3) < 0) > + exit(errno); > + setenv("SWUPDATE_INFO_FD", "3", 1); > + if (dup2(warnpipe[PIPE_WRITE], 4) < 0) > + exit(errno); > + setenv("SWUPDATE_WARN_FD", "4", 1); > + > /* close all pipes, not used anymore */ > close(stdoutpipe[PIPE_READ]); > close(stdoutpipe[PIPE_WRITE]); > close(stderrpipe[PIPE_READ]); > close(stderrpipe[PIPE_WRITE]); > + close(infopipe[PIPE_READ]); > + close(infopipe[PIPE_WRITE]); > + close(warnpipe[PIPE_READ]); > + close(warnpipe[PIPE_WRITE]); > > ret = execl("/bin/sh", "sh", "-c", cmd, (char *)NULL); > if (ret) { > @@ -300,17 +333,24 @@ int run_system_cmd(const char *cmd) > exit(1); > } > } else { > - int fds[2]; > + int fds[4]; > + int fdmax = 0; > pid_t w; > - char buf[2][SWUPDATE_GENERAL_STRING_SIZE]; > - int cindex[2] = {0, 0}; /* this is the first free char inside buffer */ > - LOGLEVEL levels[2] = { TRACELEVEL, ERRORLEVEL }; > + char buf[4][SWUPDATE_GENERAL_STRING_SIZE]; > + int cindex[4] = { 0 }; /* this is the first free char inside buffer */ > + LOGLEVEL levels[4] = { TRACELEVEL, ERRORLEVEL, INFOLEVEL, WARNLEVEL }; > > close(stdoutpipe[PIPE_WRITE]); > close(stderrpipe[PIPE_WRITE]); > + close(infopipe[PIPE_WRITE]); > + close(warnpipe[PIPE_WRITE]); > > fds[0] = stdoutpipe[PIPE_READ]; > fds[1] = stderrpipe[PIPE_READ]; > + fds[2] = infopipe[PIPE_READ]; > + fds[3] = warnpipe[PIPE_READ]; > + for (int i = 0; i < 4; i++) > + if (fds[i] > fdmax) fdmax = fds[i]; > > /* > * Use buffers (for stdout and stdin) to collect data from > @@ -319,6 +359,8 @@ int run_system_cmd(const char *cmd) > */ > memset(buf[0], 0, SWUPDATE_GENERAL_STRING_SIZE); > memset(buf[1], 0, SWUPDATE_GENERAL_STRING_SIZE); > + memset(buf[2], 0, SWUPDATE_GENERAL_STRING_SIZE); > + memset(buf[3], 0, SWUPDATE_GENERAL_STRING_SIZE); > > /* > * Now waits until the child process exits and checks > @@ -329,7 +371,7 @@ int run_system_cmd(const char *cmd) > int n1 = 0; > struct timeval tv; > fd_set readfds; > - int n, i; > + int n; > > w = waitpid(process_id, &wstatus, WNOHANG); > if (w == -1) { > @@ -347,13 +389,15 @@ int run_system_cmd(const char *cmd) > FD_ZERO(&readfds); > FD_SET(fds[0], &readfds); > FD_SET(fds[1], &readfds); > + FD_SET(fds[2], &readfds); > + FD_SET(fds[3], &readfds); > > n1 = 0; > - ret = select(max(fds[0], fds[1]) + 1, &readfds, NULL, NULL, &tv); > + ret = select(fdmax + 1, &readfds, NULL, NULL, &tv); > if (ret <= 0) > break; > > - for (i = 0; i < 2 ; i++) { > + for (int i = 0; i < 4 ; i++) { > char *pbuf = buf[i]; > int *c = &cindex[i]; > LOGLEVEL level = levels[i]; > @@ -370,7 +414,7 @@ int run_system_cmd(const char *cmd) > } while (w != process_id); > > /* print any unfinished line */ > - for (int i = 0; i < 2; i++) { > + for (int i = 0; i < 4; i++) { > if (cindex[i]) { > switch(i) { > case 0: > @@ -385,6 +429,8 @@ int run_system_cmd(const char *cmd) > > close(stdoutpipe[PIPE_READ]); > close(stderrpipe[PIPE_READ]); > + close(infopipe[PIPE_READ]); > + close(warnpipe[PIPE_READ]); > > if (WIFEXITED(wstatus)) { > ret = WEXITSTATUS(wstatus);
diff --git a/core/pctl.c b/core/pctl.c index 89ebb452de04..34bd1eef63f3 100644 --- a/core/pctl.c +++ b/core/pctl.c @@ -12,6 +12,7 @@ #include <sys/socket.h> #include <fcntl.h> #include <sys/select.h> +#include <stdlib.h> #if defined(__linux__) #include <sys/prctl.h> #endif @@ -252,6 +253,8 @@ int run_system_cmd(const char *cmd) int ret = 0; int stdoutpipe[2]; int stderrpipe[2]; + int infopipe[2]; + int warnpipe[2]; pid_t process_id; int const PIPE_READ = 0; int const PIPE_WRITE = 1; @@ -280,6 +283,23 @@ int run_system_cmd(const char *cmd) close(stdoutpipe[1]); return -EFAULT; } + if (pipe(infopipe) < 0) { + close(stderrpipe[0]); + close(stderrpipe[1]); + close(stdoutpipe[0]); + close(stdoutpipe[1]); + return -EFAULT; + } + if (pipe(warnpipe) < 0) { + close(infopipe[0]); + close(infopipe[1]); + close(stderrpipe[0]); + close(stderrpipe[1]); + close(stdoutpipe[0]); + close(stdoutpipe[1]); + return -EFAULT; + } + process_id = fork(); /* Child process, this runs the shell command */ @@ -288,11 +308,24 @@ int run_system_cmd(const char *cmd) exit(errno); if (dup2(stderrpipe[PIPE_WRITE], STDERR_FILENO) < 0) exit(errno); + /* posix sh cannot use fd >= 10, so dup these to lower + * numbers for convenience */ + if (dup2(infopipe[PIPE_WRITE], 3) < 0) + exit(errno); + setenv("SWUPDATE_INFO_FD", "3", 1); + if (dup2(warnpipe[PIPE_WRITE], 4) < 0) + exit(errno); + setenv("SWUPDATE_WARN_FD", "4", 1); + /* close all pipes, not used anymore */ close(stdoutpipe[PIPE_READ]); close(stdoutpipe[PIPE_WRITE]); close(stderrpipe[PIPE_READ]); close(stderrpipe[PIPE_WRITE]); + close(infopipe[PIPE_READ]); + close(infopipe[PIPE_WRITE]); + close(warnpipe[PIPE_READ]); + close(warnpipe[PIPE_WRITE]); ret = execl("/bin/sh", "sh", "-c", cmd, (char *)NULL); if (ret) { @@ -300,17 +333,24 @@ int run_system_cmd(const char *cmd) exit(1); } } else { - int fds[2]; + int fds[4]; + int fdmax = 0; pid_t w; - char buf[2][SWUPDATE_GENERAL_STRING_SIZE]; - int cindex[2] = {0, 0}; /* this is the first free char inside buffer */ - LOGLEVEL levels[2] = { TRACELEVEL, ERRORLEVEL }; + char buf[4][SWUPDATE_GENERAL_STRING_SIZE]; + int cindex[4] = { 0 }; /* this is the first free char inside buffer */ + LOGLEVEL levels[4] = { TRACELEVEL, ERRORLEVEL, INFOLEVEL, WARNLEVEL }; close(stdoutpipe[PIPE_WRITE]); close(stderrpipe[PIPE_WRITE]); + close(infopipe[PIPE_WRITE]); + close(warnpipe[PIPE_WRITE]); fds[0] = stdoutpipe[PIPE_READ]; fds[1] = stderrpipe[PIPE_READ]; + fds[2] = infopipe[PIPE_READ]; + fds[3] = warnpipe[PIPE_READ]; + for (int i = 0; i < 4; i++) + if (fds[i] > fdmax) fdmax = fds[i]; /* * Use buffers (for stdout and stdin) to collect data from @@ -319,6 +359,8 @@ int run_system_cmd(const char *cmd) */ memset(buf[0], 0, SWUPDATE_GENERAL_STRING_SIZE); memset(buf[1], 0, SWUPDATE_GENERAL_STRING_SIZE); + memset(buf[2], 0, SWUPDATE_GENERAL_STRING_SIZE); + memset(buf[3], 0, SWUPDATE_GENERAL_STRING_SIZE); /* * Now waits until the child process exits and checks @@ -329,7 +371,7 @@ int run_system_cmd(const char *cmd) int n1 = 0; struct timeval tv; fd_set readfds; - int n, i; + int n; w = waitpid(process_id, &wstatus, WNOHANG); if (w == -1) { @@ -347,13 +389,15 @@ int run_system_cmd(const char *cmd) FD_ZERO(&readfds); FD_SET(fds[0], &readfds); FD_SET(fds[1], &readfds); + FD_SET(fds[2], &readfds); + FD_SET(fds[3], &readfds); n1 = 0; - ret = select(max(fds[0], fds[1]) + 1, &readfds, NULL, NULL, &tv); + ret = select(fdmax + 1, &readfds, NULL, NULL, &tv); if (ret <= 0) break; - for (i = 0; i < 2 ; i++) { + for (int i = 0; i < 4 ; i++) { char *pbuf = buf[i]; int *c = &cindex[i]; LOGLEVEL level = levels[i]; @@ -370,7 +414,7 @@ int run_system_cmd(const char *cmd) } while (w != process_id); /* print any unfinished line */ - for (int i = 0; i < 2; i++) { + for (int i = 0; i < 4; i++) { if (cindex[i]) { switch(i) { case 0: @@ -385,6 +429,8 @@ int run_system_cmd(const char *cmd) close(stdoutpipe[PIPE_READ]); close(stderrpipe[PIPE_READ]); + close(infopipe[PIPE_READ]); + close(warnpipe[PIPE_READ]); if (WIFEXITED(wstatus)) { ret = WEXITSTATUS(wstatus);
subprocesses (e.g. scripts) are currently limited to ERROR and TRACE. When TRACE is not displayed (as it can get quite verbose), the only way to get messages to the user is to write to stderr which will be displayed in red as ERROR messages. This can be confusing when we just want to notify users e.g. of upcoming reboot. This commits creates a couple of additional pipes which are just handled like stdout and stderr, except they are displayed at info and warn log levels. Scripts can check for SWUPDATE_INFO_FD / SWUPDATE_WARN_FD to decide if they can output to these file descriptors. Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- core/pctl.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 8 deletions(-)