Re: [Crash-utility] [PATCH] Allows to change the error output direction

----- Original Message ----- > Hi Dave, > > Here I attach as a file for the patch. Thanks. > > Kind regards, > Daniel Hi Daniel, As I mentioned before, the concept seems reasonable, which I thought was to allow a user to prevent error() messages from being intermingled with command output by redirecting them somewhere else. But that's apparently not the case, as a few simple examples show otherwise. Here's the default setting, and a sample command generating an error: crash> set -v scroll: on (/usr/bin/less) radix: 10 (decimal) refresh: on print_max: 256 print_array: off console: (not assigned) debug: 0 core: off hash: on silent: off edit: vi namelist: /usr/lib/debug/lib/modules/3.10.0-957.21.2.el7.x86_64/vmlinux dumpfile: (null) unwind: off zero_excluded: off null-stop: off gdb: off scope: 0 (not set) offline: show redzone: on stderr: stdout crash> bt 33333 bt: invalid task or pid value: 33333 crash> Here I set stderr to /dev/null, which sets the new pc->stderr, but the behavior is still the same: crash> set stderr /dev/null stderr: /dev/null crash> set -v scroll: on (/usr/bin/less) radix: 10 (decimal) refresh: on print_max: 256 print_array: off console: (not assigned) debug: 0 core: off hash: on silent: off edit: vi namelist: /usr/lib/debug/lib/modules/3.10.0-957.21.2.el7.x86_64/vmlinux dumpfile: (null) unwind: off zero_excluded: off null-stop: off gdb: off scope: 0 (not set) offline: show redzone: on stderr: /dev/null crash> bt 33333 bt: invalid task or pid value: 33333 crash> Or if I set it to a file, the same thing happens: crash> set stderr /tmp/junk stderr: /tmp/junk crash> set -v scroll: on (/usr/bin/less) radix: 10 (decimal) refresh: on print_max: 256 print_array: off console: (not assigned) debug: 0 core: off hash: on silent: off edit: vi namelist: /usr/lib/debug/lib/modules/3.10.0-957.21.2.el7.x86_64/vmlinux dumpfile: (null) unwind: off zero_excluded: off null-stop: off gdb: off scope: 0 (not set) offline: show redzone: on stderr: /tmp/junk crash> bt 33333 bt: invalid task or pid value: 33333 crash> With your patch applied, the help page indicates: stderr stdout | fp | <path> set the direction of error put. 'stdout' always print on console. 'fp' follows the redirection or pipe command. '<path>' can be any file path in the filesystem which can save the output Is states that "<path>" can be any file path in the filesystem which can save the output. But even I redirect a command, it still doesn't seem to do what it states: crash> set stderr /dev/null stderr: /dev/null crash> bt 33333 > /tmp/junk crash> !cat /tmp/junk bt: invalid task or pid value: 33333 crash> Or if I pipe it: crash> bt 33333 | cat bt: invalid task or pid value: 33333 crash> What am I missing? And also, a couple more things... Please make pc->stderr_path a pointer: --- a/defs.h +++ b/defs.h @@ -553,6 +553,8 @@ struct program_context { ulong scope; /* optional text context address */ ulong nr_hash_queues; /* hash queue head count */ char *(*read_vmcoreinfo)(const char *); + FILE *stderr; /* error() message direction */ + char stderr_path[PATH_MAX]; /* stderr path information */ }; Since it's typically going to contain a handful of bytes, it's kind of wasteful to use PATH_MAX (4096). Just use malloc/free to get a buffer of the correct length. And please display pc->stderr and pc->stderr_path to dump_program_context() for use by "help -p". Thanks, Dave > > > On Sat, Jun 22, 2019 at 5:24 AM Dave Anderson < anderson(a)redhat.com > wrote: > > > > Hi Daniel, > > The idea seems reasonable, but the patch below is malformed: > > $ patch -p1 < error.patch > checking file defs.h > Hunk #1 FAILED at 553. > 1 out of 1 hunk FAILED > checking file help.c > patch: **** malformed patch at line 52: displayed by", > > $ > > You can see that there are a quite a few unintended line wraps > in the patch below. > > Can you make the patch a discrete attachment to your email? > > Thanks, > Dave > > > ----- Original Message ----- > > Currently, the error() is always printing the output to the console > > through 'stdout'. This does not follow redirection which is good when > > you want to know error while redirecting commands output to a file. > > However, there are situations that you want to hide error messages or > > redirect it into somewhere else. > > > > Using 'set stderr' command, it can be changed to three different > > destination - fixed 'stdout', following redirection (fp), or a custom > > file path. > > > > crash> set stderr > > stderr: stdout > > crash> sym 0x523 > /dev/null > > sym: invalid address: 0x523 > > crash> set stderr fp > > stderr: fp > > crash> sym 0x523 > /dev/null > > crash> set stderr /tmp/err.log > > stderr: /tmp/err.log > > crash> sym 0x523 > /dev/null > > crash> set stderr stdout > > stderr: stdout > > crash> sym 0x523 > /dev/null > > sym: invalid address: 0x523 > > --- > > defs.h | 2 ++ > > help.c | 5 +++++ > > main.c | 2 ++ > > tools.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- > > 4 files changed, 61 insertions(+), 3 deletions(-) > > > > diff --git a/defs.h b/defs.h > > index ccffe58..57850c6 100644 > > --- a/defs.h > > +++ b/defs.h > > @@ -553,6 +553,8 @@ struct program_context { > > ulong scope; /* optional text context address */ > > ulong nr_hash_queues; /* hash queue head count */ > > char *(*read_vmcoreinfo)(const char *); > > + FILE *stderr; /* error() message direction */ > > + char stderr_path[PATH_MAX]; /* stderr path information */ > > }; > > > > #define READMEM pc->readmem > > diff --git a/help.c b/help.c > > index 581e616..ddc8e86 100644 > > --- a/help.c > > +++ b/help.c > > @@ -1093,6 +1093,10 @@ char *help_set[] = { > > " redzone on | off if on, CONFIG_SLUB object addresses > > displayed by", > > " the kmem command will point to the > > SLAB_RED_ZONE", > > " padding inserted at the beginning of > > the object.", > > +" stderr stdout | fp | <path> set the direction of error put. > > 'stdout' always", > > +" print on console. 'fp' follows the > > redirection", > > +" or pipe command. '<path>' can be any > > file path", > > +" in the filesystem which can save the > > output", > > " ", > > " Internal variables may be set in four manners:\n", > > " 1. entering the set command in $HOME/.%src.", > > @@ -1144,6 +1148,7 @@ char *help_set[] = { > > " scope: (not set)", > > " offline: show", > > " redzone: on", > > +" stderr: stdout", > > " ", > > " Show the current context:\n", > > " %s> set", > > diff --git a/main.c b/main.c > > index 83ccd31..68bdec4 100644 > > --- a/main.c > > +++ b/main.c > > @@ -1085,6 +1085,8 @@ setup_environment(int argc, char **argv) > > * to pipes or output files. > > */ > > fp = stdout; > > + pc->stderr = stdout; > > + strcpy(pc->stderr_path, "stdout"); > > > > /* > > * Start populating the program_context structure. It's used so > > diff --git a/tools.c b/tools.c > > index 2d95c3a..840d07c 100644 > > --- a/tools.c > > +++ b/tools.c > > @@ -58,6 +58,9 @@ __error(int type, char *fmt, ...) > > void *retaddr[NUMBER_STACKFRAMES] = { 0 }; > > va_list ap; > > > > + if (!strcmp(pc->stderr_path, "fp")) > > + pc->stderr = fp; > > + > > if (CRASHDEBUG(1) || (pc->flags & DROP_CORE)) { > > SAVE_RETURN_ADDRESS(retaddr); > > console("error() trace: %lx => %lx => %lx => %lx\n", > > @@ -69,7 +72,7 @@ __error(int type, char *fmt, ...) > > va_end(ap); > > > > if (!fmt && FATAL_ERROR(type)) { > > - fprintf(stdout, "\n"); > > + fprintf(pc->stderr, "\n"); > > clean_exit(1); > > } > > > > @@ -95,14 +98,14 @@ __error(int type, char *fmt, ...) > > buf); > > fflush(pc->stdpipe); > > } else { > > - fprintf(stdout, "%s%s%s %s%s", > > + fprintf(pc->stderr, "%s%s%s %s%s", > > new_line || end_of_line ? "\n" : "", > > type == WARNING ? "WARNING" : > > type == NOTE ? "NOTE" : > > type == CONT ? spacebuf : pc->curcmd, > > type == CONT ? " " : ":", > > buf, end_of_line ? "\n" : ""); > > - fflush(stdout); > > + fflush(pc->stderr); > > } > > > > if ((fp != stdout) && (fp != pc->stdpipe) && (fp != pc->tmpfile)) { > > @@ -2483,6 +2486,51 @@ cmd_set(void) > > } > > return; > > > > + } else if (STREQ(args[optind], "stderr")) { > > + if (args[optind+1]) { > > + FILE *tmp_fp = NULL; > > + char tmp_path[PATH_MAX]; > > + > > + optind++; > > + if (STREQ(args[optind], "stdout")) { > > + tmp_fp = stdout; > > + strcpy(tmp_path, "stdout"); > > + } else if (STREQ(args[optind], "fp")) { > > + tmp_fp = fp; > > + strcpy(tmp_path, "fp"); > > + } else { > > + if (strlen(args[optind]) >= > > PATH_MAX) { > > + error(INFO, "path > > length %d is too long. (max=%d)\n", > > + > > strlen(args[optind]), PATH_MAX); > > + return; > > + } > > + tmp_fp = fopen(args[optind], "a"); > > + if (tmp_fp != NULL) { > > + strcpy(tmp_path, > > args[optind]); > > + } else { > > + error(INFO, "invalid > > path: %s\n", > > + args[optind]); > > + return; > > + } > > + > > + } > > + > > + if (strcmp(pc->stderr_path, tmp_path)) { > > + if (strcmp(pc->stderr_path, > > "stdout") > > + && strcmp(pc->stderr_path, > > "fp")) { > > + fclose(pc->stderr); > > + } > > + pc->stderr = tmp_fp; > > + strcpy(pc->stderr_path, tmp_path); > > + } > > + } > > + > > + if (runtime) { > > + fprintf(fp, "stderr: %s\n", > > + pc->stderr_path); > > + } > > + return; > > + > > } else if (XEN_HYPER_MODE()) { > > error(FATAL, "invalid argument for the Xen hypervisor\n"); > > } else if (pc->flags & MINIMAL_MODE) { > > @@ -2590,6 +2638,7 @@ show_options(void) > > fprintf(fp, "(not set)\n"); > > fprintf(fp, " offline: %s\n", pc->flags2 & OFFLINE_HIDE ? > > "hide" : "show"); > > fprintf(fp, " redzone: %s\n", pc->flags2 & REDZONE ? "on" : "off"); > > + fprintf(fp, " stderr: %s\n", pc->stderr_path); > > } > > > > > > -- > > 1.8.3.1 > > > > -- > > Crash-utility mailing list > > Crash-utility(a)redhat.com > > https://www.redhat.com/mailman/listinfo/crash-utility > > > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://www.redhat.com/mailman/listinfo/crash-utility > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://www.redhat.com/mailman/listinfo/crash-utility