On Sat, Jul 28, 2007 at 06:17:25PM +0200, Martin Pitt wrote:Hi Neil,Ahh, well then you should have nothing to worry about, this patch expands them
Neil Horman [2007-07-28 9:46 -0400]:No, I specifically mean the standard ones provided byI just want to mention a potential problem with this: If you firstI never intended for this core_pattern argument passing to be able
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.
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.
format_corename(), such as %p (pid), %s (signal), or %e (executable
name). I don't see a reason to provide additional functionality.
just fine. And none of those macros will ever have spaces in them. I suppose
it would be possible for the executable name to have spaces in it, but honestly,
thats going rather out of your way to do something that you arguably shouldn't
do anyway.
Agreed, /proc/<pid of crashing process>/* will be available while the helper appI don't think that this will be necessary. After all, the crashIn fact we considered this macro approach when doing the originalI disagree. While it might be nice to be able to specify environment
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).
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.
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).
runs.
It seems we misunderstood each other, I don't expect or want any newI think you're correct, I misundersood you previously. Apologies.
functionality in core_pattern. AN example might make it more clear:
The original problem that we are trying to solve is the currentYes, that is exactly correct.
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 containAlso correct. Thats a pretty big corner case, and I personally don't think an
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'].
executable name with spaces is/should be valid anyway, but it can be done.
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.
Jeremy asked that I make a patch next week to address split_argv's requirement
that the argc parameter be non-NULL. I'll be fixing that next week, and what I
can do is further enhance it such that it ignores spaces in quoted strings,
which should address the case that concerns you. I.E I can make split_argv
behave such that:
echo "|\"foo bar\" --pid %p" > /proc/sys/kernel/core_pattern
results in the following argv:
{{"foo bar"}, {"--pid"}, {"1234"}}
Which I think handles what you are looking for.