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

----- 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

----- Original Message ----- > Hi Dave, > > It looks like __error() function has an extra output which can cause of > confusion. I rewrote the code to cover that as well as the changes you had > asked. Please let me know how it goes. Hi Daniel, Upon initial testing, I note that your patch changes the default behavior, which is unacceptable. By default, the idea is to get all error() messages out so that they are seen by the user regardless of how the command's output may be piped or redirected. So if a command's output is redirected to a file or pipe, the error message goes both to the console as well as being intermingled in the pipe/file command output. Taking your simple example, by default, command output and error messages are piped (behind the scenes) to /usr/bin/less: crash> sym 0x523 sym: invalid address: 0x523 crash> If the default piping is turned off, command output and error messages go to stdout: crash> set scroll off scroll: off (/usr/bin/less) crash> sym 0x523 sym: invalid address: 0x523 crash> However, if the command is redirected to a file, any command output and error messages go to the file, but error messages also go to the console: crash> sym 0x523 > /tmp/output sym: invalid address: 0x523 crash> !cat /tmp/output sym: invalid address: 0x523 crash> Similarly, if the command is piped to a command, command output and error messages go to the pipe, and error messages also go to the console: crash> sym 0x523 | cat sym: invalid address: 0x523 sym: invalid address: 0x523 crash> So with your patch applied, and the new stderr variable set to the default of "stdout": crash> set stderr stderr: stdout crash> Let's run the same set of commands as above: crash> sym 0x523 sym: invalid address: 0x523 crash> set scroll off scroll: off (/usr/bin/less) crash> sym 0x523 sym: invalid address: 0x523 crash> Same behavior as always. However, if a command is redirected to a file, the error message only goes to the console, but it is not sent to the output file: crash> sym 0x523 > /tmp/output sym: invalid address: 0x523 crash> !cat /tmp/output crash> Similarly, when piped to a command, the error message is only going to one of the destinations: crash> sym 0x523 | cat sym: invalid address: 0x523 crash> So there's no way I'm going to change behavior from what it has been forever. While I didn't test your alternative settings, it's not entirely clear what you're trying to accomplish. Seemingly it would make sense to have a binary setting for the new "stderr": (1) the current default behavior, or (2) a setting allowing you to redirect all error() messages to a designated file. Option (2) would *not* send them to the console *or* intermingle them with command output. But that's just me... Also, here's a minor compiler complaint: $ make warn ... cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 main.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security main.c: In function ‘setup_environment’: main.c:1088:9: warning: implicit declaration of function ‘set_stderr’ [-Wimplicit-function-declaration] set_stderr("stdout"); ^ cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 tools.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security tools.c:42:1: warning: no previous prototype for ‘set_stderr’ [-Wmissing-prototypes] set_stderr(char *target) ^ ... Thanks, Dave > > Kind regards, > Daniel Kwon > > On Tue, Jun 25, 2019 at 2:06 AM Dave Anderson <anderson(a)redhat.com&gt; wrote: > > > > > ----- 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 > > >

----- Original Message ----- > Hi Dave, > > Yes, that is violating the default behaviour. I recheck how it should > be handled and made the below rules. > > - 'default' : Working just like the 'crash' before this 'stderr' > implementation. > - 'fp' : Only goes into one destination. It can be console in normal > command, but will go into target file if redirection or pipe is used. I still don't understand why you want to bother with this "fp" option? What's the problem you're trying to address? Dave > - '<path>' : It will go into the specified file only and no console output. > > Below is the test I have done for the test. Hope this behaviour is > reasonable. > > --------------------------- > ## default: standard error handling behaviour. > > normal command: error prints on console > > crash> set stderr default > stderr: default > > crash> sym 0x123 > sym: invalid address: 0x123 > > redirect: goes into both console and redirected file. > > crash> sym 0x123 > /tmp/output > sym: invalid address: 0x123 > > crash> !cat /tmp/output > sym: invalid address: 0x123 > > > pipe: goes into both console and piped direction. > > crash> sym 0x123 | cat > sym: invalid address: 0x123 > sym: invalid address: 0x123 > > crash> sym 0x123 | cat > /tmp/output > sym: invalid address: 0x123 > > crash> !cat /tmp/output > sym: invalid address: 0x123 > > > > > ## fp: Only to the one target such as stdout, pipe, or redirected file > > normal command: error prints on console > > crash> set stderr fp > stderr: fp > > crash> sym 0x123 > sym: invalid address: 0x123 > > redirect: goes into redirected file only. > > crash> sym 0x123 > /tmp/output > crash> !cat /tmp/output > sym: invalid address: 0x123 > > > pipe: goes into piped direction only. > > crash> sym 0x123 | cat > sym: invalid address: 0x123 > > crash> sym 0x123 | cat > /tmp/output > > crash> !cat /tmp/output > sym: invalid address: 0x123 > > > > ## <file path>: Only to the specified file. > > normal command: error goes into the specified file only. > > crash> set stderr /tmp/stderr > stderr: /tmp/stderr > crash> sym 0x123 > crash> !cat /tmp/stderr > sym: invalid address: 0x123 > > redirect: error goes into the specified file only. > > crash> sym 0x124 > /tmp/output > crash> !cat /tmp/output > crash> !cat /tmp/stderr > sym: invalid address: 0x123 > sym: invalid address: 0x124 > > pipe: error goes into the specified file only. > > crash> sym 0x125 | cat > crash> sym 0x126 | cat > /tmp/output > crash> !cat /tmp/output > crash> !cat /tmp/stderr > sym: invalid address: 0x123 > sym: invalid address: 0x124 > sym: invalid address: 0x125 > sym: invalid address: 0x126 > --------------------------- > > Regards, > Daniel Kwon > > Kind regards, > > Daniel Kwon, RHCA > > Principle Software Maintenance Engineer, CEE > > Red Hat APAC > > > > On Fri, Jun 28, 2019 at 5:27 AM Dave Anderson <anderson(a)redhat.com&gt; wrote: > > > > > > > > ----- Original Message ----- > > > Hi Dave, > > > > > > It looks like __error() function has an extra output which can cause of > > > confusion. I rewrote the code to cover that as well as the changes you > > > had > > > asked. Please let me know how it goes. > > > > Hi Daniel, > > > > Upon initial testing, I note that your patch changes the default behavior, > > which is unacceptable. > > > > By default, the idea is to get all error() messages out so that they are > > seen by the user regardless of how the command's output may be piped or > > redirected. So if a command's output is redirected to a file or pipe, > > the error message goes both to the console as well as being intermingled > > in the pipe/file command output. > > > > Taking your simple example, by default, command output and error messages > > are piped (behind the scenes) to /usr/bin/less: > > > > crash> sym 0x523 > > sym: invalid address: 0x523 > > crash> > > > > If the default piping is turned off, command output and error messages > > go to stdout: > > > > crash> set scroll off > > scroll: off (/usr/bin/less) > > crash> sym 0x523 > > sym: invalid address: 0x523 > > crash> > > > > However, if the command is redirected to a file, any command output and > > error > > messages go to the file, but error messages also go to the console: > > > > crash> sym 0x523 > /tmp/output > > sym: invalid address: 0x523 > > crash> !cat /tmp/output > > sym: invalid address: 0x523 > > crash> > > > > Similarly, if the command is piped to a command, command output and error > > messages > > go to the pipe, and error messages also go to the console: > > > > crash> sym 0x523 | cat > > sym: invalid address: 0x523 > > sym: invalid address: 0x523 > > crash> > > > > So with your patch applied, and the new stderr variable set to the default > > of "stdout": > > > > crash> set stderr > > stderr: stdout > > crash> > > > > Let's run the same set of commands as above: > > > > crash> sym 0x523 > > sym: invalid address: 0x523 > > crash> set scroll off > > scroll: off (/usr/bin/less) > > crash> sym 0x523 > > sym: invalid address: 0x523 > > crash> > > > > Same behavior as always. However, if a command is redirected to a file, > > the error message only goes to the console, but it is not sent to the > > output file: > > > > crash> sym 0x523 > /tmp/output > > sym: invalid address: 0x523 > > crash> !cat /tmp/output > > crash> > > > > Similarly, when piped to a command, the error message is only going to > > one of the destinations: > > > > crash> sym 0x523 | cat > > sym: invalid address: 0x523 > > crash> > > > > So there's no way I'm going to change behavior from what it has > > been forever. > > > > While I didn't test your alternative settings, it's not entirely clear > > what you're trying to accomplish. Seemingly it would make sense to have > > a binary setting for the new "stderr": > > > > (1) the current default behavior, or > > (2) a setting allowing you to redirect all error() messages to a > > designated file. > > > > Option (2) would *not* send them to the console *or* intermingle them > > with command output. But that's just me... > > > > Also, here's a minor compiler complaint: > > > > $ make warn > > ... > > cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 main.c -Wall -O2 > > -Wstrict-prototypes -Wmissing-prototypes -fstack-protector > > -Wformat-security > > main.c: In function ‘setup_environment’: > > main.c:1088:9: warning: implicit declaration of function ‘set_stderr’ > > [-Wimplicit-function-declaration] > > set_stderr("stdout"); > > ^ > > cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 tools.c -Wall -O2 > > -Wstrict-prototypes -Wmissing-prototypes -fstack-protector > > -Wformat-security > > tools.c:42:1: warning: no previous prototype for ‘set_stderr’ > > [-Wmissing-prototypes] > > set_stderr(char *target) > > ^ > > ... > > > > Thanks, > > Dave > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Kind regards, > > > Daniel Kwon > > > > > > On Tue, Jun 25, 2019 at 2:06 AM Dave Anderson <anderson(a)redhat.com&gt; > > > wrote: > > > > > > > > > > > ----- 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 > > > > > > > >

----- Original Message ----- > I am writing an extension based on 'mpykdump' and calling > 'exec_crash_command()' to find symbol details as shown below. > > --- > def find_symbol(str): > try: > sym = exec_crash_command("sym %s" % str) > if sym.startswith("sym:") != True: > return " <" + "".join(sym.split()[2:]) + ">" > except: > pass > > return "" > --- > > In the below example, I am trying to interpret address > '0xffff92a3fba0f100' by calling 'sym' crash command, but can't handle > the situation where the symbol doesn't exist. > > --- > ... > 0xffffffff92858b75 <do_sys_poll+741>: callq 0xffffffff92986f80 <__x86_indirect_thunk_rdx> > sym: invalid address: 0xffff92a3fba0f100 > 0xffffffff92858b7a <do_sys_poll+746>: mov 0x50(%rsp),%rcx ;0xffff92a3fba0f100 > ... > --- > > By using 'fp', I can redirect the error into the > 'exec_crash_command()' result which I can use to identify the reason > if there's error and can avoid showing unnecessary error on console. If you'll allow me to continue beating a dead horse... I've never used mpykdump before, and it's confusing because you are showing disassembly output, with a "sym" command error message appearing in the middle. Are you calling excec_crash_command() while you are parsing disassembly output from within some other mypkdump execution stream? Anyway, are you saying that -- with the current behavior -- when you redirect the exec_crash_command() function, that the sym error messages do *not* go to the redirected file or pipe stream? It sounds like you're saying that your proposed "fp" setting would simply not display the extra "unnecessary" error message on the console. Dave > > Regards, > Daniel > > Kind regards, > > Daniel Kwon, RHCA > > Principle Software Maintenance Engineer, CEE > > Red Hat APAC > > > > On Fri, Jun 28, 2019 at 11:41 PM Dave Anderson <anderson(a)redhat.com&gt; wrote: > > > > > > > > ----- Original Message ----- > > > Hi Dave, > > > > > > Yes, that is violating the default behaviour. I recheck how it should > > > be handled and made the below rules. > > > > > > - 'default' : Working just like the 'crash' before this 'stderr' > > > implementation. > > > - 'fp' : Only goes into one destination. It can be console in normal > > > command, but will go into target file if redirection or pipe is used. > > > > I still don't understand why you want to bother with this "fp" option? > > What's the problem you're trying to address? > > > > Dave > > > > > > > - '<path>' : It will go into the specified file only and no console > > > output. > > > > > > Below is the test I have done for the test. Hope this behaviour is > > > reasonable. > > > > > > --------------------------- > > > ## default: standard error handling behaviour. > > > > > > normal command: error prints on console > > > > > > crash> set stderr default > > > stderr: default > > > > > > crash> sym 0x123 > > > sym: invalid address: 0x123 > > > > > > redirect: goes into both console and redirected file. > > > > > > crash> sym 0x123 > /tmp/output > > > sym: invalid address: 0x123 > > > > > > crash> !cat /tmp/output > > > sym: invalid address: 0x123 > > > > > > > > > pipe: goes into both console and piped direction. > > > > > > crash> sym 0x123 | cat > > > sym: invalid address: 0x123 > > > sym: invalid address: 0x123 > > > > > > crash> sym 0x123 | cat > /tmp/output > > > sym: invalid address: 0x123 > > > > > > crash> !cat /tmp/output > > > sym: invalid address: 0x123 > > > > > > > > > > > > > > > ## fp: Only to the one target such as stdout, pipe, or redirected file > > > > > > normal command: error prints on console > > > > > > crash> set stderr fp > > > stderr: fp > > > > > > crash> sym 0x123 > > > sym: invalid address: 0x123 > > > > > > redirect: goes into redirected file only. > > > > > > crash> sym 0x123 > /tmp/output > > > crash> !cat /tmp/output > > > sym: invalid address: 0x123 > > > > > > > > > pipe: goes into piped direction only. > > > > > > crash> sym 0x123 | cat > > > sym: invalid address: 0x123 > > > > > > crash> sym 0x123 | cat > /tmp/output > > > > > > crash> !cat /tmp/output > > > sym: invalid address: 0x123 > > > > > > > > > > > > ## <file path>: Only to the specified file. > > > > > > normal command: error goes into the specified file only. > > > > > > crash> set stderr /tmp/stderr > > > stderr: /tmp/stderr > > > crash> sym 0x123 > > > crash> !cat /tmp/stderr > > > sym: invalid address: 0x123 > > > > > > redirect: error goes into the specified file only. > > > > > > crash> sym 0x124 > /tmp/output > > > crash> !cat /tmp/output > > > crash> !cat /tmp/stderr > > > sym: invalid address: 0x123 > > > sym: invalid address: 0x124 > > > > > > pipe: error goes into the specified file only. > > > > > > crash> sym 0x125 | cat > > > crash> sym 0x126 | cat > /tmp/output > > > crash> !cat /tmp/output > > > crash> !cat /tmp/stderr > > > sym: invalid address: 0x123 > > > sym: invalid address: 0x124 > > > sym: invalid address: 0x125 > > > sym: invalid address: 0x126 > > > --------------------------- > > > > > > Regards, > > > Daniel Kwon > > > > > > Kind regards, > > > > > > Daniel Kwon, RHCA > > > > > > Principle Software Maintenance Engineer, CEE > > > > > > Red Hat APAC > > > > > > > > > > > > On Fri, Jun 28, 2019 at 5:27 AM Dave Anderson <anderson(a)redhat.com&gt; > > > wrote: > > > > > > > > > > > > > > > > ----- Original Message ----- > > > > > Hi Dave, > > > > > > > > > > It looks like __error() function has an extra output which can cause > > > > > of > > > > > confusion. I rewrote the code to cover that as well as the changes > > > > > you > > > > > had > > > > > asked. Please let me know how it goes. > > > > > > > > Hi Daniel, > > > > > > > > Upon initial testing, I note that your patch changes the default > > > > behavior, > > > > which is unacceptable. > > > > > > > > By default, the idea is to get all error() messages out so that they > > > > are > > > > seen by the user regardless of how the command's output may be piped or > > > > redirected. So if a command's output is redirected to a file or pipe, > > > > the error message goes both to the console as well as being > > > > intermingled > > > > in the pipe/file command output. > > > > > > > > Taking your simple example, by default, command output and error > > > > messages > > > > are piped (behind the scenes) to /usr/bin/less: > > > > > > > > crash> sym 0x523 > > > > sym: invalid address: 0x523 > > > > crash> > > > > > > > > If the default piping is turned off, command output and error messages > > > > go to stdout: > > > > > > > > crash> set scroll off > > > > scroll: off (/usr/bin/less) > > > > crash> sym 0x523 > > > > sym: invalid address: 0x523 > > > > crash> > > > > > > > > However, if the command is redirected to a file, any command output and > > > > error > > > > messages go to the file, but error messages also go to the console: > > > > > > > > crash> sym 0x523 > /tmp/output > > > > sym: invalid address: 0x523 > > > > crash> !cat /tmp/output > > > > sym: invalid address: 0x523 > > > > crash> > > > > > > > > Similarly, if the command is piped to a command, command output and > > > > error > > > > messages > > > > go to the pipe, and error messages also go to the console: > > > > > > > > crash> sym 0x523 | cat > > > > sym: invalid address: 0x523 > > > > sym: invalid address: 0x523 > > > > crash> > > > > > > > > So with your patch applied, and the new stderr variable set to the > > > > default > > > > of "stdout": > > > > > > > > crash> set stderr > > > > stderr: stdout > > > > crash> > > > > > > > > Let's run the same set of commands as above: > > > > > > > > crash> sym 0x523 > > > > sym: invalid address: 0x523 > > > > crash> set scroll off > > > > scroll: off (/usr/bin/less) > > > > crash> sym 0x523 > > > > sym: invalid address: 0x523 > > > > crash> > > > > > > > > Same behavior as always. However, if a command is redirected to a > > > > file, > > > > the error message only goes to the console, but it is not sent to the > > > > output file: > > > > > > > > crash> sym 0x523 > /tmp/output > > > > sym: invalid address: 0x523 > > > > crash> !cat /tmp/output > > > > crash> > > > > > > > > Similarly, when piped to a command, the error message is only going to > > > > one of the destinations: > > > > > > > > crash> sym 0x523 | cat > > > > sym: invalid address: 0x523 > > > > crash> > > > > > > > > So there's no way I'm going to change behavior from what it has > > > > been forever. > > > > > > > > While I didn't test your alternative settings, it's not entirely clear > > > > what you're trying to accomplish. Seemingly it would make sense to > > > > have > > > > a binary setting for the new "stderr": > > > > > > > > (1) the current default behavior, or > > > > (2) a setting allowing you to redirect all error() messages to a > > > > designated file. > > > > > > > > Option (2) would *not* send them to the console *or* intermingle them > > > > with command output. But that's just me... > > > > > > > > Also, here's a minor compiler complaint: > > > > > > > > $ make warn > > > > ... > > > > cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 main.c -Wall -O2 > > > > -Wstrict-prototypes -Wmissing-prototypes -fstack-protector > > > > -Wformat-security > > > > main.c: In function ‘setup_environment’: > > > > main.c:1088:9: warning: implicit declaration of function ‘set_stderr’ > > > > [-Wimplicit-function-declaration] > > > > set_stderr("stdout"); > > > > ^ > > > > cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 tools.c -Wall -O2 > > > > -Wstrict-prototypes -Wmissing-prototypes -fstack-protector > > > > -Wformat-security > > > > tools.c:42:1: warning: no previous prototype for ‘set_stderr’ > > > > [-Wmissing-prototypes] > > > > set_stderr(char *target) > > > > ^ > > > > ... > > > > > > > > Thanks, > > > > Dave > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Kind regards, > > > > > Daniel Kwon > > > > > > > > > > On Tue, Jun 25, 2019 at 2:06 AM Dave Anderson <anderson(a)redhat.com&gt; > > > > > wrote: > > > > > > > > > > > > > > > > > ----- 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 > > > > > > > > > > > > > > >

----- Original Message ----- > I shouldn't use that message in the previous email. Yes, I am writing > a command that does extra actions while doing 'dis' which gives you > some extra information to make it easy to understand the code. As a > part of it, it is trying to find symbols for any given values. > > Anyway, here I put another example that just does calling 'sym' > command in exec_crash_command(). You can try it once load 'mpykdump' > shared object. Let me ask another question. Does mpykdump allow consumers of the epython command to take advantage of all of the exported crash utility functions that could be used in this case, e.g., value_symbol()? Or does it restrict users to simply being able to run and parse the output of that exec_crash_command() function?

Dave > > ----------------------------------------------------------------------------------- > $ pwd > /root/Work > $ cat test_cmd.py > from pykdump.API import * > > def test_cmd(): > try: > sym = exec_crash_command("sym 0x123") > print("{%s}" % sym) > except: > pass > > > if ( __name__ == '__main__'): > test_cmd() > > $ ./crash > ... > crash> extend /root/.crash.d/mpykdump.so > /root/.crash.d/mpykdump.so: shared object loaded > > crash> set stderr default > stderr: default > crash> epython /root/Work/test_cmd.py > sym: invalid address: 0x123 > {sym: invalid address: 0x123 > } > > ** Execution took 0.00s (real) 0.00s (CPU) > crash> set stderr fp > stderr: fp > crash> epython /root/Work/test_cmd.py > {sym: invalid address: 0x123 > } > > ** Execution took 0.00s (real) 0.00s (CPU) > crash> set stderr /tmp/output > stderr: /tmp/output > crash> epython /root/Work/test_cmd.py > crash> !cat /tmp/output > sym: invalid address: 0x123 > ----------------------------------------------------------------------------------- > > Yes, in the above by using 'fp', we can redirect the error message > into the same exec_crash_command() and prevent showing any messages on > console. By doing that, we can have a full control for the messages. > If we want to seperate error messages from the beginning, but not > making duplicate, we could use file path instead. > > PS. I am attaching a new patch that has a small change as there was a > memory leak when file open is failed. > > Regards, > Daniel Kwon > > On Sun, Jun 30, 2019 at 1:42 AM Dave Anderson <anderson(a)redhat.com&gt; wrote: > > > > > > > > ----- Original Message ----- > > > I am writing an extension based on 'mpykdump' and calling > > > 'exec_crash_command()' to find symbol details as shown below. > > > > > > --- > > > def find_symbol(str): > > > try: > > > sym = exec_crash_command("sym %s" % str) > > > if sym.startswith("sym:") != True: > > > return " <" + "".join(sym.split()[2:]) + ">" > > > except: > > > pass > > > > > > return "" > > > --- > > > > > > In the below example, I am trying to interpret address > > > '0xffff92a3fba0f100' by calling 'sym' crash command, but can't handle > > > the situation where the symbol doesn't exist. > > > > > > --- > > > ... > > > 0xffffffff92858b75 <do_sys_poll+741>: callq 0xffffffff92986f80 > > > <__x86_indirect_thunk_rdx> > > > sym: invalid address: 0xffff92a3fba0f100 > > > 0xffffffff92858b7a <do_sys_poll+746>: mov 0x50(%rsp),%rcx > > > ;0xffff92a3fba0f100 > > > ... > > > --- > > > > > > By using 'fp', I can redirect the error into the > > > 'exec_crash_command()' result which I can use to identify the reason > > > if there's error and can avoid showing unnecessary error on console. > > > > If you'll allow me to continue beating a dead horse... > > > > I've never used mpykdump before, and it's confusing because you are > > showing > > disassembly > > output, with a "sym" command error message appearing in the middle. Are > > you calling > > excec_crash_command() while you are parsing disassembly output from > > within > > some other > > mypkdump execution stream? > > > > Anyway, are you saying that -- with the current behavior -- when you > > redirect the exec_crash_command() > > function, that the sym error messages do *not* go to the redirected file > > or > > pipe stream? > > > > It sounds like you're saying that your proposed "fp" setting would simply > > not display the extra > > "unnecessary" error message on the console. > > > > Dave > > > > > > > > > > Regards, > > > Daniel > > > > > > Kind regards, > > > > > > Daniel Kwon, RHCA > > > > > > Principle Software Maintenance Engineer, CEE > > > > > > Red Hat APAC > > > > > > > > > > > > On Fri, Jun 28, 2019 at 11:41 PM Dave Anderson <anderson(a)redhat.com&gt; > > > wrote: > > > > > > > > > > > > > > > > ----- Original Message ----- > > > > > Hi Dave, > > > > > > > > > > Yes, that is violating the default behaviour. I recheck how it > > > > > should > > > > > be handled and made the below rules. > > > > > > > > > > - 'default' : Working just like the 'crash' before this 'stderr' > > > > > implementation. > > > > > - 'fp' : Only goes into one destination. It can be console in > > > > > normal > > > > > command, but will go into target file if redirection or pipe is > > > > > used. > > > > > > > > I still don't understand why you want to bother with this "fp" > > > > option? > > > > What's the problem you're trying to address? > > > > > > > > Dave > > > > > > > > > > > > > - '<path>' : It will go into the specified file only and no console > > > > > output. > > > > > > > > > > Below is the test I have done for the test. Hope this behaviour is > > > > > reasonable. > > > > > > > > > > --------------------------- > > > > > ## default: standard error handling behaviour. > > > > > > > > > > normal command: error prints on console > > > > > > > > > > crash> set stderr default > > > > > stderr: default > > > > > > > > > > crash> sym 0x123 > > > > > sym: invalid address: 0x123 > > > > > > > > > > redirect: goes into both console and redirected file. > > > > > > > > > > crash> sym 0x123 > /tmp/output > > > > > sym: invalid address: 0x123 > > > > > > > > > > crash> !cat /tmp/output > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > > > > pipe: goes into both console and piped direction. > > > > > > > > > > crash> sym 0x123 | cat > > > > > sym: invalid address: 0x123 > > > > > sym: invalid address: 0x123 > > > > > > > > > > crash> sym 0x123 | cat > /tmp/output > > > > > sym: invalid address: 0x123 > > > > > > > > > > crash> !cat /tmp/output > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > > > > > > > > > > > > > > ## fp: Only to the one target such as stdout, pipe, or redirected > > > > > file > > > > > > > > > > normal command: error prints on console > > > > > > > > > > crash> set stderr fp > > > > > stderr: fp > > > > > > > > > > crash> sym 0x123 > > > > > sym: invalid address: 0x123 > > > > > > > > > > redirect: goes into redirected file only. > > > > > > > > > > crash> sym 0x123 > /tmp/output > > > > > crash> !cat /tmp/output > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > > > > pipe: goes into piped direction only. > > > > > > > > > > crash> sym 0x123 | cat > > > > > sym: invalid address: 0x123 > > > > > > > > > > crash> sym 0x123 | cat > /tmp/output > > > > > > > > > > crash> !cat /tmp/output > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > > > > > > > > > ## <file path>: Only to the specified file. > > > > > > > > > > normal command: error goes into the specified file only. > > > > > > > > > > crash> set stderr /tmp/stderr > > > > > stderr: /tmp/stderr > > > > > crash> sym 0x123 > > > > > crash> !cat /tmp/stderr > > > > > sym: invalid address: 0x123 > > > > > > > > > > redirect: error goes into the specified file only. > > > > > > > > > > crash> sym 0x124 > /tmp/output > > > > > crash> !cat /tmp/output > > > > > crash> !cat /tmp/stderr > > > > > sym: invalid address: 0x123 > > > > > sym: invalid address: 0x124 > > > > > > > > > > pipe: error goes into the specified file only. > > > > > > > > > > crash> sym 0x125 | cat > > > > > crash> sym 0x126 | cat > /tmp/output > > > > > crash> !cat /tmp/output > > > > > crash> !cat /tmp/stderr > > > > > sym: invalid address: 0x123 > > > > > sym: invalid address: 0x124 > > > > > sym: invalid address: 0x125 > > > > > sym: invalid address: 0x126 > > > > > --------------------------- > > > > > > > > > > Regards, > > > > > Daniel Kwon > > > > > > > > > > Kind regards, > > > > > > > > > > Daniel Kwon, RHCA > > > > > > > > > > Principle Software Maintenance Engineer, CEE > > > > > > > > > > Red Hat APAC > > > > > > > > > > > > > > > > > > > > On Fri, Jun 28, 2019 at 5:27 AM Dave Anderson <anderson(a)redhat.com&gt; > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > ----- Original Message ----- > > > > > > > Hi Dave, > > > > > > > > > > > > > > It looks like __error() function has an extra output which can > > > > > > > cause > > > > > > > of > > > > > > > confusion. I rewrote the code to cover that as well as the > > > > > > > changes > > > > > > > you > > > > > > > had > > > > > > > asked. Please let me know how it goes. > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > Upon initial testing, I note that your patch changes the default > > > > > > behavior, > > > > > > which is unacceptable. > > > > > > > > > > > > By default, the idea is to get all error() messages out so that > > > > > > they > > > > > > are > > > > > > seen by the user regardless of how the command's output may be > > > > > > piped or > > > > > > redirected. So if a command's output is redirected to a file or > > > > > > pipe, > > > > > > the error message goes both to the console as well as being > > > > > > intermingled > > > > > > in the pipe/file command output. > > > > > > > > > > > > Taking your simple example, by default, command output and error > > > > > > messages > > > > > > are piped (behind the scenes) to /usr/bin/less: > > > > > > > > > > > > crash> sym 0x523 > > > > > > sym: invalid address: 0x523 > > > > > > crash> > > > > > > > > > > > > If the default piping is turned off, command output and error > > > > > > messages > > > > > > go to stdout: > > > > > > > > > > > > crash> set scroll off > > > > > > scroll: off (/usr/bin/less) > > > > > > crash> sym 0x523 > > > > > > sym: invalid address: 0x523 > > > > > > crash> > > > > > > > > > > > > However, if the command is redirected to a file, any command > > > > > > output > > > > > > and > > > > > > error > > > > > > messages go to the file, but error messages also go to the > > > > > > console: > > > > > > > > > > > > crash> sym 0x523 > /tmp/output > > > > > > sym: invalid address: 0x523 > > > > > > crash> !cat /tmp/output > > > > > > sym: invalid address: 0x523 > > > > > > crash> > > > > > > > > > > > > Similarly, if the command is piped to a command, command output > > > > > > and > > > > > > error > > > > > > messages > > > > > > go to the pipe, and error messages also go to the console: > > > > > > > > > > > > crash> sym 0x523 | cat > > > > > > sym: invalid address: 0x523 > > > > > > sym: invalid address: 0x523 > > > > > > crash> > > > > > > > > > > > > So with your patch applied, and the new stderr variable set to > > > > > > the > > > > > > default > > > > > > of "stdout": > > > > > > > > > > > > crash> set stderr > > > > > > stderr: stdout > > > > > > crash> > > > > > > > > > > > > Let's run the same set of commands as above: > > > > > > > > > > > > crash> sym 0x523 > > > > > > sym: invalid address: 0x523 > > > > > > crash> set scroll off > > > > > > scroll: off (/usr/bin/less) > > > > > > crash> sym 0x523 > > > > > > sym: invalid address: 0x523 > > > > > > crash> > > > > > > > > > > > > Same behavior as always. However, if a command is redirected to > > > > > > a > > > > > > file, > > > > > > the error message only goes to the console, but it is not sent to > > > > > > the > > > > > > output file: > > > > > > > > > > > > crash> sym 0x523 > /tmp/output > > > > > > sym: invalid address: 0x523 > > > > > > crash> !cat /tmp/output > > > > > > crash> > > > > > > > > > > > > Similarly, when piped to a command, the error message is only > > > > > > going > > > > > > to > > > > > > one of the destinations: > > > > > > > > > > > > crash> sym 0x523 | cat > > > > > > sym: invalid address: 0x523 > > > > > > crash> > > > > > > > > > > > > So there's no way I'm going to change behavior from what it has > > > > > > been forever. > > > > > > > > > > > > While I didn't test your alternative settings, it's not entirely > > > > > > clear > > > > > > what you're trying to accomplish. Seemingly it would make sense > > > > > > to > > > > > > have > > > > > > a binary setting for the new "stderr": > > > > > > > > > > > > (1) the current default behavior, or > > > > > > (2) a setting allowing you to redirect all error() messages to a > > > > > > designated file. > > > > > > > > > > > > Option (2) would *not* send them to the console *or* intermingle > > > > > > them > > > > > > with command output. But that's just me... > > > > > > > > > > > > Also, here's a minor compiler complaint: > > > > > > > > > > > > $ make warn > > > > > > ... > > > > > > cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 main.c -Wall -O2 > > > > > > -Wstrict-prototypes -Wmissing-prototypes -fstack-protector > > > > > > -Wformat-security > > > > > > main.c: In function ‘setup_environment’: > > > > > > main.c:1088:9: warning: implicit declaration of function > > > > > > ‘set_stderr’ > > > > > > [-Wimplicit-function-declaration] > > > > > > set_stderr("stdout"); > > > > > > ^ > > > > > > cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 tools.c -Wall -O2 > > > > > > -Wstrict-prototypes -Wmissing-prototypes -fstack-protector > > > > > > -Wformat-security > > > > > > tools.c:42:1: warning: no previous prototype for ‘set_stderr’ > > > > > > [-Wmissing-prototypes] > > > > > > set_stderr(char *target) > > > > > > ^ > > > > > > ... > > > > > > > > > > > > Thanks, > > > > > > Dave > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Kind regards, > > > > > > > Daniel Kwon > > > > > > > > > > > > > > On Tue, Jun 25, 2019 at 2:06 AM Dave Anderson > > > > > > > <anderson(a)redhat.com&gt; > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > ----- 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 > > > > > > > > > > > > > > > > > > > > > > > > -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility

I shouldn't use that message in the previous email. Yes, I am writing a command that does extra actions while doing 'dis' which gives you some extra information to make it easy to understand the code. As a part of it, it is trying to find symbols for any given values. Anyway, here I put another example that just does calling 'sym' command in exec_crash_command(). You can try it once load 'mpykdump' shared object. ----------------------------------------------------------------------------------- $ pwd /root/Work $ cat test_cmd.py from pykdump.API import * def test_cmd(): try: sym = exec_crash_command("sym 0x123") print("{%s}" % sym) except: pass if ( __name__ == '__main__'): test_cmd() $ ./crash ... crash> extend /root/.crash.d/mpykdump.so /root/.crash.d/mpykdump.so: shared object loaded crash> set stderr default stderr: default crash> epython /root/Work/test_cmd.py sym: invalid address: 0x123 {sym: invalid address: 0x123 } ** Execution took 0.00s (real) 0.00s (CPU) crash> set stderr fp stderr: fp crash> epython /root/Work/test_cmd.py {sym: invalid address: 0x123 } ** Execution took 0.00s (real) 0.00s (CPU) crash> set stderr /tmp/output stderr: /tmp/output crash> epython /root/Work/test_cmd.py crash> !cat /tmp/output sym: invalid address: 0x123 ----------------------------------------------------------------------------------- Yes, in the above by using 'fp', we can redirect the error message into the same exec_crash_command() and prevent showing any messages on console. By doing that, we can have a full control for the messages. If we want to seperate error messages from the beginning, but not making duplicate, we could use file path instead. PS. I am attaching a new patch that has a small change as there was a memory leak when file open is failed. Regards, Daniel Kwon On Sun, Jun 30, 2019 at 1:42 AM Dave Anderson <anderson(a)redhat.com&gt; wrote: > > > > ----- Original Message ----- > > I am writing an extension based on 'mpykdump' and calling > > 'exec_crash_command()' to find symbol details as shown below. > > > > --- > > def find_symbol(str): > > try: > > sym = exec_crash_command("sym %s" % str) > > if sym.startswith("sym:") != True: > > return " <" + "".join(sym.split()[2:]) + ">" > > except: > > pass > > > > return "" > > --- > > > > In the below example, I am trying to interpret address > > '0xffff92a3fba0f100' by calling 'sym' crash command, but can't handle > > the situation where the symbol doesn't exist. > > > > --- > > ... > > 0xffffffff92858b75 <do_sys_poll+741>: callq 0xffffffff92986f80 > > <__x86_indirect_thunk_rdx> > > sym: invalid address: 0xffff92a3fba0f100 > > 0xffffffff92858b7a <do_sys_poll+746>: mov 0x50(%rsp),%rcx > > ;0xffff92a3fba0f100 > > ... > > --- > > > > By using 'fp', I can redirect the error into the > > 'exec_crash_command()' result which I can use to identify the reason > > if there's error and can avoid showing unnecessary error on console. > > If you'll allow me to continue beating a dead horse... > > I've never used mpykdump before, and it's confusing because you are showing > disassembly > output, with a "sym" command error message appearing in the middle. Are > you calling > excec_crash_command() while you are parsing disassembly output from within > some other > mypkdump execution stream? > > Anyway, are you saying that -- with the current behavior -- when you > redirect the exec_crash_command() > function, that the sym error messages do *not* go to the redirected file or > pipe stream? > > It sounds like you're saying that your proposed "fp" setting would simply > not display the extra > "unnecessary" error message on the console. > > Dave > > > > > > Regards, > > Daniel > > > > Kind regards, > > > > Daniel Kwon, RHCA > > > > Principle Software Maintenance Engineer, CEE > > > > Red Hat APAC > > > > > > > > On Fri, Jun 28, 2019 at 11:41 PM Dave Anderson <anderson(a)redhat.com&gt; > > wrote: > > > > > > > > > > > > ----- Original Message ----- > > > > Hi Dave, > > > > > > > > Yes, that is violating the default behaviour. I recheck how it should > > > > be handled and made the below rules. > > > > > > > > - 'default' : Working just like the 'crash' before this 'stderr' > > > > implementation. > > > > - 'fp' : Only goes into one destination. It can be console in normal > > > > command, but will go into target file if redirection or pipe is used. > > > > > > I still don't understand why you want to bother with this "fp" option? > > > What's the problem you're trying to address? > > > > > > Dave > > > > > > > > > > - '<path>' : It will go into the specified file only and no console > > > > output. > > > > > > > > Below is the test I have done for the test. Hope this behaviour is > > > > reasonable. > > > > > > > > --------------------------- > > > > ## default: standard error handling behaviour. > > > > > > > > normal command: error prints on console > > > > > > > > crash> set stderr default > > > > stderr: default > > > > > > > > crash> sym 0x123 > > > > sym: invalid address: 0x123 > > > > > > > > redirect: goes into both console and redirected file. > > > > > > > > crash> sym 0x123 > /tmp/output > > > > sym: invalid address: 0x123 > > > > > > > > crash> !cat /tmp/output > > > > sym: invalid address: 0x123 > > > > > > > > > > > > pipe: goes into both console and piped direction. > > > > > > > > crash> sym 0x123 | cat > > > > sym: invalid address: 0x123 > > > > sym: invalid address: 0x123 > > > > > > > > crash> sym 0x123 | cat > /tmp/output > > > > sym: invalid address: 0x123 > > > > > > > > crash> !cat /tmp/output > > > > sym: invalid address: 0x123 > > > > > > > > > > > > > > > > > > > > ## fp: Only to the one target such as stdout, pipe, or redirected > > > > file > > > > > > > > normal command: error prints on console > > > > > > > > crash> set stderr fp > > > > stderr: fp > > > > > > > > crash> sym 0x123 > > > > sym: invalid address: 0x123 > > > > > > > > redirect: goes into redirected file only. > > > > > > > > crash> sym 0x123 > /tmp/output > > > > crash> !cat /tmp/output > > > > sym: invalid address: 0x123 > > > > > > > > > > > > pipe: goes into piped direction only. > > > > > > > > crash> sym 0x123 | cat > > > > sym: invalid address: 0x123 > > > > > > > > crash> sym 0x123 | cat > /tmp/output > > > > > > > > crash> !cat /tmp/output > > > > sym: invalid address: 0x123 > > > > > > > > > > > > > > > > ## <file path>: Only to the specified file. > > > > > > > > normal command: error goes into the specified file only. > > > > > > > > crash> set stderr /tmp/stderr > > > > stderr: /tmp/stderr > > > > crash> sym 0x123 > > > > crash> !cat /tmp/stderr > > > > sym: invalid address: 0x123 > > > > > > > > redirect: error goes into the specified file only. > > > > > > > > crash> sym 0x124 > /tmp/output > > > > crash> !cat /tmp/output > > > > crash> !cat /tmp/stderr > > > > sym: invalid address: 0x123 > > > > sym: invalid address: 0x124 > > > > > > > > pipe: error goes into the specified file only. > > > > > > > > crash> sym 0x125 | cat > > > > crash> sym 0x126 | cat > /tmp/output > > > > crash> !cat /tmp/output > > > > crash> !cat /tmp/stderr > > > > sym: invalid address: 0x123 > > > > sym: invalid address: 0x124 > > > > sym: invalid address: 0x125 > > > > sym: invalid address: 0x126 > > > > --------------------------- > > > > > > > > Regards, > > > > Daniel Kwon > > > > > > > > Kind regards, > > > > > > > > Daniel Kwon, RHCA > > > > > > > > Principle Software Maintenance Engineer, CEE > > > > > > > > Red Hat APAC > > > > > > > > > > > > > > > > On Fri, Jun 28, 2019 at 5:27 AM Dave Anderson <anderson(a)redhat.com&gt; > > > > wrote: > > > > > > > > > > > > > > > > > > > > ----- Original Message ----- > > > > > > Hi Dave, > > > > > > > > > > > > It looks like __error() function has an extra output which can > > > > > > cause > > > > > > of > > > > > > confusion. I rewrote the code to cover that as well as the > > > > > > changes > > > > > > you > > > > > > had > > > > > > asked. Please let me know how it goes. > > > > > > > > > > Hi Daniel, > > > > > > > > > > Upon initial testing, I note that your patch changes the default > > > > > behavior, > > > > > which is unacceptable. > > > > > > > > > > By default, the idea is to get all error() messages out so that > > > > > they > > > > > are > > > > > seen by the user regardless of how the command's output may be > > > > > piped or > > > > > redirected. So if a command's output is redirected to a file or > > > > > pipe, > > > > > the error message goes both to the console as well as being > > > > > intermingled > > > > > in the pipe/file command output. > > > > > > > > > > Taking your simple example, by default, command output and error > > > > > messages > > > > > are piped (behind the scenes) to /usr/bin/less: > > > > > > > > > > crash> sym 0x523 > > > > > sym: invalid address: 0x523 > > > > > crash> > > > > > > > > > > If the default piping is turned off, command output and error > > > > > messages > > > > > go to stdout: > > > > > > > > > > crash> set scroll off > > > > > scroll: off (/usr/bin/less) > > > > > crash> sym 0x523 > > > > > sym: invalid address: 0x523 > > > > > crash> > > > > > > > > > > However, if the command is redirected to a file, any command output > > > > > and > > > > > error > > > > > messages go to the file, but error messages also go to the console: > > > > > > > > > > crash> sym 0x523 > /tmp/output > > > > > sym: invalid address: 0x523 > > > > > crash> !cat /tmp/output > > > > > sym: invalid address: 0x523 > > > > > crash> > > > > > > > > > > Similarly, if the command is piped to a command, command output and > > > > > error > > > > > messages > > > > > go to the pipe, and error messages also go to the console: > > > > > > > > > > crash> sym 0x523 | cat > > > > > sym: invalid address: 0x523 > > > > > sym: invalid address: 0x523 > > > > > crash> > > > > > > > > > > So with your patch applied, and the new stderr variable set to the > > > > > default > > > > > of "stdout": > > > > > > > > > > crash> set stderr > > > > > stderr: stdout > > > > > crash> > > > > > > > > > > Let's run the same set of commands as above: > > > > > > > > > > crash> sym 0x523 > > > > > sym: invalid address: 0x523 > > > > > crash> set scroll off > > > > > scroll: off (/usr/bin/less) > > > > > crash> sym 0x523 > > > > > sym: invalid address: 0x523 > > > > > crash> > > > > > > > > > > Same behavior as always. However, if a command is redirected to a > > > > > file, > > > > > the error message only goes to the console, but it is not sent to > > > > > the > > > > > output file: > > > > > > > > > > crash> sym 0x523 > /tmp/output > > > > > sym: invalid address: 0x523 > > > > > crash> !cat /tmp/output > > > > > crash> > > > > > > > > > > Similarly, when piped to a command, the error message is only going > > > > > to > > > > > one of the destinations: > > > > > > > > > > crash> sym 0x523 | cat > > > > > sym: invalid address: 0x523 > > > > > crash> > > > > > > > > > > So there's no way I'm going to change behavior from what it has > > > > > been forever. > > > > > > > > > > While I didn't test your alternative settings, it's not entirely > > > > > clear > > > > > what you're trying to accomplish. Seemingly it would make sense to > > > > > have > > > > > a binary setting for the new "stderr": > > > > > > > > > > (1) the current default behavior, or > > > > > (2) a setting allowing you to redirect all error() messages to a > > > > > designated file. > > > > > > > > > > Option (2) would *not* send them to the console *or* intermingle > > > > > them > > > > > with command output. But that's just me... > > > > > > > > > > Also, here's a minor compiler complaint: > > > > > > > > > > $ make warn > > > > > ... > > > > > cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 main.c -Wall -O2 > > > > > -Wstrict-prototypes -Wmissing-prototypes -fstack-protector > > > > > -Wformat-security > > > > > main.c: In function ‘setup_environment’: > > > > > main.c:1088:9: warning: implicit declaration of function > > > > > ‘set_stderr’ > > > > > [-Wimplicit-function-declaration] > > > > > set_stderr("stdout"); > > > > > ^ > > > > > cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 tools.c -Wall -O2 > > > > > -Wstrict-prototypes -Wmissing-prototypes -fstack-protector > > > > > -Wformat-security > > > > > tools.c:42:1: warning: no previous prototype for ‘set_stderr’ > > > > > [-Wmissing-prototypes] > > > > > set_stderr(char *target) > > > > > ^ > > > > > ... > > > > > > > > > > Thanks, > > > > > Dave > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Kind regards, > > > > > > Daniel Kwon > > > > > > > > > > > > On Tue, Jun 25, 2019 at 2:06 AM Dave Anderson > > > > > > <anderson(a)redhat.com&gt; > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > ----- 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 > > > > > > > > > > > > > > > > > > >

Hi Dave, There was a missed corner case that it doesn't work if scroll is on. The new patch is passed both scroll on and off. First, this doesn't work as you've advertised: > > crash> set stderr /dev/null > stderr: /dev/null > crash> sym 555 > sym: invalid address: 555 > crash> The new patch applied you had mentioned. I was also unhappy with name 'fp'. > > Secondly, could you change the name of the "stderr" variable? It's > kind of confusing, especially since the crash internals specifically > avoid using stderr. Perhaps use "error" instead? And then with respect > to the 3 possible options, "fp" is confusing and somewhat meaningless > to a user -- let's use "redirect", since that's what you're describing. > > Internally, please clarify/rename the "pc->stderr" and "pc->stderr_path" > member > names to be "pc->error_fp" and "pc->error_path". And in > dump_program_context(), > display both members explicitly as such, not the way you are displaying > the stderr FILE pointer as the stderr_path string: > > + fprintf(fp, " stderr: %s\n", pc->stderr_path); > > In the case of 'redirect', we have to update file pointer on the fly as it can have different value from the default stdout or current fp. > At the beginning of __error(), you've got this: > > + if (!strcmp(pc->stderr_path, "fp")) > + pc->stderr = fp; > + > > I don't understand that. What's going on there? > > Applied STREQ() for those locations. Kind regards, Daniel Kwon > And lastly, just for my own sanity, in all the places where you're > using strcmp() and !strcmp(), can you please just use STREQ() instead? > > Thanks, > Dave > > > > > > ----- Original Message ----- > > I shouldn't use that message in the previous email. Yes, I am writing > > a command that does extra actions while doing 'dis' which gives you > > some extra information to make it easy to understand the code. As a > > part of it, it is trying to find symbols for any given values. > > > > Anyway, here I put another example that just does calling 'sym' > > command in exec_crash_command(). You can try it once load 'mpykdump' > > shared object. > > > > > ----------------------------------------------------------------------------------- > > $ pwd > > /root/Work > > $ cat test_cmd.py > > from pykdump.API import * > > > > def test_cmd(): > > try: > > sym = exec_crash_command("sym 0x123") > > print("{%s}" % sym) > > except: > > pass > > > > > > if ( __name__ == '__main__'): > > test_cmd() > > > > $ ./crash > > ... > > crash> extend /root/.crash.d/mpykdump.so > > /root/.crash.d/mpykdump.so: shared object loaded > > > > crash> set stderr default > > stderr: default > > crash> epython /root/Work/test_cmd.py > > sym: invalid address: 0x123 > > {sym: invalid address: 0x123 > > } > > > > ** Execution took 0.00s (real) 0.00s (CPU) > > crash> set stderr fp > > stderr: fp > > crash> epython /root/Work/test_cmd.py > > {sym: invalid address: 0x123 > > } > > > > ** Execution took 0.00s (real) 0.00s (CPU) > > crash> set stderr /tmp/output > > stderr: /tmp/output > > crash> epython /root/Work/test_cmd.py > > crash> !cat /tmp/output > > sym: invalid address: 0x123 > > > ----------------------------------------------------------------------------------- > > > > Yes, in the above by using 'fp', we can redirect the error message > > into the same exec_crash_command() and prevent showing any messages on > > console. By doing that, we can have a full control for the messages. > > If we want to seperate error messages from the beginning, but not > > making duplicate, we could use file path instead. > > > > PS. I am attaching a new patch that has a small change as there was a > > memory leak when file open is failed. > > > > Regards, > > Daniel Kwon > > > > On Sun, Jun 30, 2019 at 1:42 AM Dave Anderson <anderson(a)redhat.com&gt; > wrote: > > > > > > > > > > > > ----- Original Message ----- > > > > I am writing an extension based on 'mpykdump' and calling > > > > 'exec_crash_command()' to find symbol details as shown below. > > > > > > > > --- > > > > def find_symbol(str): > > > > try: > > > > sym = exec_crash_command("sym %s" % str) > > > > if sym.startswith("sym:") != True: > > > > return " <" + "".join(sym.split()[2:]) + ">" > > > > except: > > > > pass > > > > > > > > return "" > > > > --- > > > > > > > > In the below example, I am trying to interpret address > > > > '0xffff92a3fba0f100' by calling 'sym' crash command, but can't handle > > > > the situation where the symbol doesn't exist. > > > > > > > > --- > > > > ... > > > > 0xffffffff92858b75 <do_sys_poll+741>: callq 0xffffffff92986f80 > > > > <__x86_indirect_thunk_rdx> > > > > sym: invalid address: 0xffff92a3fba0f100 > > > > 0xffffffff92858b7a <do_sys_poll+746>: mov 0x50(%rsp),%rcx > > > > ;0xffff92a3fba0f100 > > > > ... > > > > --- > > > > > > > > By using 'fp', I can redirect the error into the > > > > 'exec_crash_command()' result which I can use to identify the reason > > > > if there's error and can avoid showing unnecessary error on console. > > > > > > If you'll allow me to continue beating a dead horse... > > > > > > I've never used mpykdump before, and it's confusing because you are > showing > > > disassembly > > > output, with a "sym" command error message appearing in the middle. > Are > > > you calling > > > excec_crash_command() while you are parsing disassembly output from > within > > > some other > > > mypkdump execution stream? > > > > > > Anyway, are you saying that -- with the current behavior -- when you > > > redirect the exec_crash_command() > > > function, that the sym error messages do *not* go to the redirected > file or > > > pipe stream? > > > > > > It sounds like you're saying that your proposed "fp" setting would > simply > > > not display the extra > > > "unnecessary" error message on the console. > > > > > > Dave > > > > > > > > > > > > > > Regards, > > > > Daniel > > > > > > > > Kind regards, > > > > > > > > Daniel Kwon, RHCA > > > > > > > > Principle Software Maintenance Engineer, CEE > > > > > > > > Red Hat APAC > > > > > > > > > > > > > > > > On Fri, Jun 28, 2019 at 11:41 PM Dave Anderson <anderson(a)redhat.com&gt; > > > > wrote: > > > > > > > > > > > > > > > > > > > > ----- Original Message ----- > > > > > > Hi Dave, > > > > > > > > > > > > Yes, that is violating the default behaviour. I recheck how it > should > > > > > > be handled and made the below rules. > > > > > > > > > > > > - 'default' : Working just like the 'crash' before this 'stderr' > > > > > > implementation. > > > > > > - 'fp' : Only goes into one destination. It can be console in > normal > > > > > > command, but will go into target file if redirection or pipe is > used. > > > > > > > > > > I still don't understand why you want to bother with this "fp" > option? > > > > > What's the problem you're trying to address? > > > > > > > > > > Dave > > > > > > > > > > > > > > > > - '<path>' : It will go into the specified file only and no > console > > > > > > output. > > > > > > > > > > > > Below is the test I have done for the test. Hope this behaviour > is > > > > > > reasonable. > > > > > > > > > > > > --------------------------- > > > > > > ## default: standard error handling behaviour. > > > > > > > > > > > > normal command: error prints on console > > > > > > > > > > > > crash> set stderr default > > > > > > stderr: default > > > > > > > > > > > > crash> sym 0x123 > > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > redirect: goes into both console and redirected file. > > > > > > > > > > > > crash> sym 0x123 > /tmp/output > > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > crash> !cat /tmp/output > > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > > > > > > > pipe: goes into both console and piped direction. > > > > > > > > > > > > crash> sym 0x123 | cat > > > > > > sym: invalid address: 0x123 > > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > crash> sym 0x123 | cat > /tmp/output > > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > crash> !cat /tmp/output > > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ## fp: Only to the one target such as stdout, pipe, or redirected > > > > > > file > > > > > > > > > > > > normal command: error prints on console > > > > > > > > > > > > crash> set stderr fp > > > > > > stderr: fp > > > > > > > > > > > > crash> sym 0x123 > > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > redirect: goes into redirected file only. > > > > > > > > > > > > crash> sym 0x123 > /tmp/output > > > > > > crash> !cat /tmp/output > > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > > > > > > > pipe: goes into piped direction only. > > > > > > > > > > > > crash> sym 0x123 | cat > > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > crash> sym 0x123 | cat > /tmp/output > > > > > > > > > > > > crash> !cat /tmp/output > > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > > > > > > > > > > > > > ## <file path>: Only to the specified file. > > > > > > > > > > > > normal command: error goes into the specified file only. > > > > > > > > > > > > crash> set stderr /tmp/stderr > > > > > > stderr: /tmp/stderr > > > > > > crash> sym 0x123 > > > > > > crash> !cat /tmp/stderr > > > > > > sym: invalid address: 0x123 > > > > > > > > > > > > redirect: error goes into the specified file only. > > > > > > > > > > > > crash> sym 0x124 > /tmp/output > > > > > > crash> !cat /tmp/output > > > > > > crash> !cat /tmp/stderr > > > > > > sym: invalid address: 0x123 > > > > > > sym: invalid address: 0x124 > > > > > > > > > > > > pipe: error goes into the specified file only. > > > > > > > > > > > > crash> sym 0x125 | cat > > > > > > crash> sym 0x126 | cat > /tmp/output > > > > > > crash> !cat /tmp/output > > > > > > crash> !cat /tmp/stderr > > > > > > sym: invalid address: 0x123 > > > > > > sym: invalid address: 0x124 > > > > > > sym: invalid address: 0x125 > > > > > > sym: invalid address: 0x126 > > > > > > --------------------------- > > > > > > > > > > > > Regards, > > > > > > Daniel Kwon > > > > > > > > > > > > Kind regards, > > > > > > > > > > > > Daniel Kwon, RHCA > > > > > > > > > > > > Principle Software Maintenance Engineer, CEE > > > > > > > > > > > > Red Hat APAC > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Jun 28, 2019 at 5:27 AM Dave Anderson < > anderson(a)redhat.com&gt; > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > ----- Original Message ----- > > > > > > > > Hi Dave, > > > > > > > > > > > > > > > > It looks like __error() function has an extra output which > can > > > > > > > > cause > > > > > > > > of > > > > > > > > confusion. I rewrote the code to cover that as well as the > > > > > > > > changes > > > > > > > > you > > > > > > > > had > > > > > > > > asked. Please let me know how it goes. > > > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > Upon initial testing, I note that your patch changes the > default > > > > > > > behavior, > > > > > > > which is unacceptable. > > > > > > > > > > > > > > By default, the idea is to get all error() messages out so that > > > > > > > they > > > > > > > are > > > > > > > seen by the user regardless of how the command's output may be > > > > > > > piped or > > > > > > > redirected. So if a command's output is redirected to a file > or > > > > > > > pipe, > > > > > > > the error message goes both to the console as well as being > > > > > > > intermingled > > > > > > > in the pipe/file command output. > > > > > > > > > > > > > > Taking your simple example, by default, command output and > error > > > > > > > messages > > > > > > > are piped (behind the scenes) to /usr/bin/less: > > > > > > > > > > > > > > crash> sym 0x523 > > > > > > > sym: invalid address: 0x523 > > > > > > > crash> > > > > > > > > > > > > > > If the default piping is turned off, command output and error > > > > > > > messages > > > > > > > go to stdout: > > > > > > > > > > > > > > crash> set scroll off > > > > > > > scroll: off (/usr/bin/less) > > > > > > > crash> sym 0x523 > > > > > > > sym: invalid address: 0x523 > > > > > > > crash> > > > > > > > > > > > > > > However, if the command is redirected to a file, any command > output > > > > > > > and > > > > > > > error > > > > > > > messages go to the file, but error messages also go to the > console: > > > > > > > > > > > > > > crash> sym 0x523 > /tmp/output > > > > > > > sym: invalid address: 0x523 > > > > > > > crash> !cat /tmp/output > > > > > > > sym: invalid address: 0x523 > > > > > > > crash> > > > > > > > > > > > > > > Similarly, if the command is piped to a command, command > output and > > > > > > > error > > > > > > > messages > > > > > > > go to the pipe, and error messages also go to the console: > > > > > > > > > > > > > > crash> sym 0x523 | cat > > > > > > > sym: invalid address: 0x523 > > > > > > > sym: invalid address: 0x523 > > > > > > > crash> > > > > > > > > > > > > > > So with your patch applied, and the new stderr variable set to > the > > > > > > > default > > > > > > > of "stdout": > > > > > > > > > > > > > > crash> set stderr > > > > > > > stderr: stdout > > > > > > > crash> > > > > > > > > > > > > > > Let's run the same set of commands as above: > > > > > > > > > > > > > > crash> sym 0x523 > > > > > > > sym: invalid address: 0x523 > > > > > > > crash> set scroll off > > > > > > > scroll: off (/usr/bin/less) > > > > > > > crash> sym 0x523 > > > > > > > sym: invalid address: 0x523 > > > > > > > crash> > > > > > > > > > > > > > > Same behavior as always. However, if a command is redirected > to a > > > > > > > file, > > > > > > > the error message only goes to the console, but it is not sent > to > > > > > > > the > > > > > > > output file: > > > > > > > > > > > > > > crash> sym 0x523 > /tmp/output > > > > > > > sym: invalid address: 0x523 > > > > > > > crash> !cat /tmp/output > > > > > > > crash> > > > > > > > > > > > > > > Similarly, when piped to a command, the error message is only > going > > > > > > > to > > > > > > > one of the destinations: > > > > > > > > > > > > > > crash> sym 0x523 | cat > > > > > > > sym: invalid address: 0x523 > > > > > > > crash> > > > > > > > > > > > > > > So there's no way I'm going to change behavior from what it has > > > > > > > been forever. > > > > > > > > > > > > > > While I didn't test your alternative settings, it's not > entirely > > > > > > > clear > > > > > > > what you're trying to accomplish. Seemingly it would make > sense to > > > > > > > have > > > > > > > a binary setting for the new "stderr": > > > > > > > > > > > > > > (1) the current default behavior, or > > > > > > > (2) a setting allowing you to redirect all error() messages > to a > > > > > > > designated file. > > > > > > > > > > > > > > Option (2) would *not* send them to the console *or* > intermingle > > > > > > > them > > > > > > > with command output. But that's just me... > > > > > > > > > > > > > > Also, here's a minor compiler complaint: > > > > > > > > > > > > > > $ make warn > > > > > > > ... > > > > > > > cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 main.c -Wall -O2 > > > > > > > -Wstrict-prototypes -Wmissing-prototypes -fstack-protector > > > > > > > -Wformat-security > > > > > > > main.c: In function ‘setup_environment’: > > > > > > > main.c:1088:9: warning: implicit declaration of function > > > > > > > ‘set_stderr’ > > > > > > > [-Wimplicit-function-declaration] > > > > > > > set_stderr("stdout"); > > > > > > > ^ > > > > > > > cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 tools.c -Wall -O2 > > > > > > > -Wstrict-prototypes -Wmissing-prototypes -fstack-protector > > > > > > > -Wformat-security > > > > > > > tools.c:42:1: warning: no previous prototype for ‘set_stderr’ > > > > > > > [-Wmissing-prototypes] > > > > > > > set_stderr(char *target) > > > > > > > ^ > > > > > > > ... > > > > > > > > > > > > > > Thanks, > > > > > > > Dave > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Kind regards, > > > > > > > > Daniel Kwon > > > > > > > > > > > > > > > > On Tue, Jun 25, 2019 at 2:06 AM Dave Anderson > > > > > > > > <anderson(a)redhat.com&gt; > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > ----- 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >