Re: [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 26 Jul 2007 15:31:45 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:

> On Thu, Jul 26, 2007 at 11:48:32AM -0700, Andrew Morton wrote:
> > On Thu, 26 Jul 2007 13:40:19 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:
> > 
> > > 	Currently, core dumps can be redirected to a pipe by placing the
> > > following string template in /proc/sys/kernel/core_pattern:
> > > |<path/to/application>
> > > This patch extends this ability, allowing the core_pattern to contain arguments
> > > to be passed as an argv array to the userspace helper application.  It also add
> > > a format specifier, %c, which allows the RLIM_CORE value of the crashing
> > > application to be passed on the command line, since RLIMIT_CORE is reduced to
> > > zero when execing the userspace helper
> > 
> > This all seems to be getting a bit nutty.  Who needs this feature
> > and what will they do with it, etc?
> > 
> Why nutty?  Ubuntu has had apport for a bit now, which monitors the system for
> crashed process and attempts to help the user auto-file a bugreport with a
> relevant distro based on its configuration.  Ubuntu has implemented lots of
> their functionality with some patches that they never pushed upstream (and IMHO,
> have some security issues).  This is my attempt to do what their doing sanely,
> so the other distro's (primarily fedora) can take advantage of this technology.
> Will Woods (who has been copied on this set of patches) can give you more detail
> if you like.   As for this specific feature, I wanted to include it for the same
> reason that I mentioned above.  core_pattern, when set to a pipe, reduces
> RLIMIT_CORE to zero.  This patch lets you pass the real core limit value
> directly to the application in the event it wants to save the core to disk, but
> still wishes to respect the original ulimit value.

Somewhere in there is a changelog trying to get out.

It's quite important to explan things like this: what the feature is *for*,
why we want it in Linux, what it value is to our users, stuff like that. 
Things with which we can justify the additional overhead/maintenance cost,
etc.

So please, include a full changelog in v2?

> > You have a few open-coded kstrdup()s in there, btw.  And an open-coded
> > free_argv_array() on the error path.  And a lot of checkpatch warnings.
> I don't see what you're referring to.  free_argv_array is only called if it was
> initially setup (keyed of the same ispipe variable), and all the memory for the
> strcpy's in format_corename_argv is unwound and freed in the out_free label of
> the same function on error.  If there is somethign I'm missing, please let me
> know, I'm happy to fix it.

The cleanup code at out_free() could, I think, just be a call to
free_argv_array().

> I know its been a large number of patches over the last few days, and I'm sorry
> for that.

heh.  More than 100 patches is getting largeish.

>  I've been trying to break this up into a sane amount of discrete
> chunks that don't depend on one another.  If you would prefer that I roll them
> up into a larger patch, I'm happy to do that as well.  Just let me know.

Well if you have mutiple patches hitting on the same area it would be
better to present it as a sequence-number patch series which all tells a
nice story.  Start out describing the end-point and what value it all adds,
follow that up with the various bits of implementation.

Dribbling in various related patches at one-per-day makes it all a bit hard
to follow and manage.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/