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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux