[Crash-utility] [PATCH] Allows to change the error output direction
Daniel Kwon
dkwon at redhat.com
Mon Jul 15 04:48:05 UTC 2019
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 at redhat.com>
> 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 at redhat.com>
> > > > 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 at redhat.com>
> > > > > > 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 at redhat.com>
> > > > > > > > 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 at 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 at redhat.com
> > > > > > > > > > > https://www.redhat.com/mailman/listinfo/crash-utility
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > Crash-utility mailing list
> > > > > > > > > > Crash-utility at redhat.com
> > > > > > > > > > https://www.redhat.com/mailman/listinfo/crash-utility
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > Crash-utility mailing list
> > > > > > > > > > Crash-utility at redhat.com
> > > > > > > > > > https://www.redhat.com/mailman/listinfo/crash-utility
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20190715/355944d2/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Allows-to-change-the-error-output-direction.patch
Type: application/octet-stream
Size: 8683 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20190715/355944d2/attachment.obj>
More information about the Crash-utility
mailing list