Messages in this thread | | | Date | Sat, 28 Jul 2007 18:17:25 +0200 | From | Martin Pitt <> | Subject | Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe |
| |
Hi Neil,
Neil Horman [2007-07-28 9:46 -0400]: > > I just want to mention a potential problem with this: If you first > > expand the macros (from pattern to corename) and then split > > corename into an argv, then this breaks macro expansions > > containing spaces. This mostly affects the executable name, of > > course. > > > I never intended for this core_pattern argument passing to be able > to expand macros, other than the macros specified by the > core_pattern code. If you want it to do that, we can address that > with a later patch.
No, I specifically mean the standard ones provided by format_corename(), such as %p (pid), %s (signal), or %e (executable name). I don't see a reason to provide additional functionality.
> > In fact we considered this macro approach when doing the original > > patches in the Ubuntu kernel, but we eventually used environment > > variables because they are much easier and more robust to > > implement than doing a robust macro expansion (i. e. first split > > core_pattern into an argv and then call the macro expansion for > > each element). > > > I disagree. While it might be nice to be able to specify environment > variables as command line arguments, it would be much easier to just > let the core_pattern executable inherit the crashing processes > environment. we don't do that currently, but we easily could. That > way any information that the crashing process wants the dying > process to know can be inherited and vetted easily by apport (or > whatever the core_pattern points to). I'll do a patch later for > that if you don't like it.
I don't think that this will be necessary. After all, the crash handler can read all the environment from /proc/<pid>/ (and that's indeed what apport does to figure out relevant parts from the environment like the locale).
It seems we misunderstood each other, I don't expect or want any new functionality in core_pattern. AN example might make it more clear:
The original problem that we are trying to solve is the current behaviour of core_pattern expansion with pipes:
|/bin/crash --pid %p
would try to execute the program '/bin/crash --pid 1234' instead of calling /bin/crash with ['--pid', '1234'] as argv, right? Your patch achieves the latter by splitting the formatted core dump string into an array (at spaces).
I pointed out that this leads to problems when macro values contain spaces. This currently affects hostname (%h) (although this really should not happen in practice) and executable name (%e) (rare, but at least valid). I. e. for an executable name "foo bar" your patch would expand
|/bin/crash %e
to ['/bin/crash', 'foo', 'bar'] instead of ['/bin/crash', 'foo bar'].
Of course this is a corner case, and personally I don't really care. I strive to keep the assumptions about the interface at a minimum, so right now Apport's only required input is the core dump itself (over stdin); signal and pid can be read from the environment, and if not present, they are read from the core dump.
I did not defend Ubuntu's usage of environment variables, on the contrary. Using the standard macros is more explicit and elegant, and I welcome that change. I just pointed out the reason why we chose the environment variable approach initially.
I just wanted to mention this little problem for the sake of correctness.
Thank you, and have a nice weekend!
Martin
-- Martin Pitt http://www.piware.de Ubuntu Developer http://www.ubuntu.com Debian Developer http://www.debian.org [unhandled content-type:application/pgp-signature] | |