----- 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@xxxxxxxxxx> 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@xxxxxxxxxx >
> > 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@xxxxxxxxxx
> > > > https://www.redhat.com/mailman/listinfo/crash-utility
> > > >
> > >
> > > --
> > > Crash-utility mailing list
> > > Crash-utility@xxxxxxxxxx
> > > https://www.redhat.com/mailman/listinfo/crash-utility
> > >
> > > --
> > > Crash-utility mailing list
> > > Crash-utility@xxxxxxxxxx
> > > https://www.redhat.com/mailman/listinfo/crash-utility
> >
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
