Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Sun, Jul 29, 2007 at 09:03:29PM +0800, Eugene Teo wrote:
> Neil Horman wrote:
> [...]
> > + /* core limit size */
> > + case 'c':
> > + rc = snprintf(out_ptr, out_end - out_ptr,
> > + "%lu",
> > current->signal->rlim[RLIMIT_CORE].rlim_cur);
>
> Trailing space.
>
> [...]
> > - if(call_usermodehelper_pipe(corename+1, NULL, NULL, )) {
> > + if(call_usermodehelper_pipe(corename+1, helper_argv, NULL,
> > )) {
>^
>
> Use tabs, and a missing space after '('.
>
I think your reader is screwing up the patch. The version that I sent, which I
have here, shows that as all tabs, no spaces.
As for the '(', yeah, that looks like its been there for awhile. I'll clean it
all up when I address macro expansion, as per the conversation Martin and I have
been holding.
Regards
Neil
> Eugene
--
/***
*Neil Horman
*Software Engineer
*Red Hat, Inc.
[EMAIL PROTECTED]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
Neil Horman wrote:
[...]
> + /* core limit size */
> + case 'c':
> + rc = snprintf(out_ptr, out_end - out_ptr,
> + "%lu",
> current->signal->rlim[RLIMIT_CORE].rlim_cur);
Trailing space.
[...]
> - if(call_usermodehelper_pipe(corename+1, NULL, NULL, )) {
> + if(call_usermodehelper_pipe(corename+1, helper_argv, NULL,
> )) {
^
Use tabs, and a missing space after '('.
Eugene
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Sun, Jul 29, 2007 at 11:34:18AM +0200, Martin Pitt wrote:
> Hi Neil,
>
> Neil Horman [2007-07-28 13:21 -0400]:
> > 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.
>
> Oh, handling escaping and quoting is going to make it fairly
> complicated, but sure, if you need that for other things, too, that
> would solve the remaining case. I just wonder if, instead of
> implementing escaping, it wouldn't be easier to first split on spaces
> and then escape macros?
>
If it were just this case, then yes. But we also need to make this work
properly for when core_pattern is not a pipe. So the macro expansion routine
needs to be generalized somewhat. Certainly doable, it just takes a bit more
testing and care in coding. I'll start looking into it in the upcomming week.
> > Thank you for clearing me up on this. So it would seem we're ok with what
> > we
> > have now, correct?
>
> Absolutely, yes.
>
> > We just have a potential corner case to address, which I can
> > reasonably handle with a modification to split_argv, that I have a
> > todo on next week.
>
> Right, it's really just for perfectionism. Spaces in executable names
> are EBW anyway, and readlink()ing /proc//exe is much more robust
> anyway in terms of a small and orthogonal interface.
>
Understood.
> If the upstream kernel guys don't worry about it and consider it a
> blocker for merging, I don't either. :-)
>
Copy that
Thanks & Regards
Neil
> Thanks a lot,
>
> Martin
>
> --
> Martin Pitthttp://www.piware.de
> Ubuntu Developer http://www.ubuntu.com
> Debian Developer http://www.debian.org
--
/***
*Neil Horman
*Software Engineer
*Red Hat, Inc.
[EMAIL PROTECTED]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Sun, Jul 29, 2007 at 02:23:10PM +0530, Aneesh Kumar K.V wrote:
>
>
> Neil Horman wrote:
> >On Sat, Jul 28, 2007 at 06:17:25PM +0200, Martin Pitt wrote:
> >>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.
> >>
> >Ahh, well then you should have nothing to worry about, this patch expands
> >them
> >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.
> >
> 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// (and that's
> >>indeed what apport does to figure out relevant parts from the
> >>environment like the locale).
> >>
> >Agreed, /proc//* will be available while the
> >helper app
> >runs.
> >
> >>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:
> >>
> >I think you're correct, I misundersood you previously. Apologies.
> >
> >>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).
> >>
> >Yes, that is exactly correct.
> >
> >>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'].
> >>
> >Also correct. Thats a pretty big corner case, and I personally don't
> >think an
> >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.
> >
>
>
> One would also need to quote the expansion of %e as Martin listed in the
> previous mail
> So a %e should expand to \"my executable\". So that it get passed as single
> argument.
>
> I guess we should only do it when "|" is specified in core pattern.
> Otherwise we would
> have core file as
>
> core."my
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 13:21 -0400]:
> 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.
Oh, handling escaping and quoting is going to make it fairly
complicated, but sure, if you need that for other things, too, that
would solve the remaining case. I just wonder if, instead of
implementing escaping, it wouldn't be easier to first split on spaces
and then escape macros?
> Thank you for clearing me up on this. So it would seem we're ok with what we
> have now, correct?
Absolutely, yes.
> We just have a potential corner case to address, which I can
> reasonably handle with a modification to split_argv, that I have a
> todo on next week.
Right, it's really just for perfectionism. Spaces in executable names
are EBW anyway, and readlink()ing /proc//exe is much more robust
anyway in terms of a small and orthogonal interface.
If the upstream kernel guys don't worry about it and consider it a
blocker for merging, I don't either. :-)
Thanks a lot,
Martin
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
signature.asc
Description: Digital signature
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
Neil Horman wrote:
On Sat, Jul 28, 2007 at 06:17:25PM +0200, Martin Pitt wrote:
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.
Ahh, well then you should have nothing to worry about, this patch expands them
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.
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// (and that's
indeed what apport does to figure out relevant parts from the
environment like the locale).
Agreed, /proc//* will be available while the helper app
runs.
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:
I think you're correct, I misundersood you previously. Apologies.
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).
Yes, that is exactly correct.
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'].
Also correct. Thats a pretty big corner case, and I personally don't think an
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.
One would also need to quote the expansion of %e as Martin listed in the
previous mail
So a %e should expand to \"my executable\". So that it get passed as single
argument.
I guess we should only do it when "|" is specified in core pattern. Otherwise
we would
have core file as
core."my exectutable"
-aneesh
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
Neil Horman wrote:
On Sat, Jul 28, 2007 at 06:17:25PM +0200, Martin Pitt wrote:
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.
Ahh, well then you should have nothing to worry about, this patch expands them
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.
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).
Agreed, /proc/pid of crashing process/* will be available while the helper app
runs.
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:
I think you're correct, I misundersood you previously. Apologies.
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).
Yes, that is exactly correct.
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'].
Also correct. Thats a pretty big corner case, and I personally don't think an
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.
One would also need to quote the expansion of %e as Martin listed in the
previous mail
So a %e should expand to \my executable\. So that it get passed as single
argument.
I guess we should only do it when | is specified in core pattern. Otherwise
we would
have core file as
core.my exectutable
-aneesh
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 13:21 -0400]:
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.
Oh, handling escaping and quoting is going to make it fairly
complicated, but sure, if you need that for other things, too, that
would solve the remaining case. I just wonder if, instead of
implementing escaping, it wouldn't be easier to first split on spaces
and then escape macros?
Thank you for clearing me up on this. So it would seem we're ok with what we
have now, correct?
Absolutely, yes.
We just have a potential corner case to address, which I can
reasonably handle with a modification to split_argv, that I have a
todo on next week.
Right, it's really just for perfectionism. Spaces in executable names
are EBW anyway, and readlink()ing /proc/pid/exe is much more robust
anyway in terms of a small and orthogonal interface.
If the upstream kernel guys don't worry about it and consider it a
blocker for merging, I don't either. :-)
Thanks a lot,
Martin
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
signature.asc
Description: Digital signature
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Sun, Jul 29, 2007 at 02:23:10PM +0530, Aneesh Kumar K.V wrote:
Neil Horman wrote:
On Sat, Jul 28, 2007 at 06:17:25PM +0200, Martin Pitt wrote:
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.
Ahh, well then you should have nothing to worry about, this patch expands
them
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.
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).
Agreed, /proc/pid of crashing process/* will be available while the
helper app
runs.
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:
I think you're correct, I misundersood you previously. Apologies.
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).
Yes, that is exactly correct.
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'].
Also correct. Thats a pretty big corner case, and I personally don't
think an
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.
One would also need to quote the expansion of %e as Martin listed in the
previous mail
So a %e should expand to \my executable\. So that it get passed as single
argument.
I guess we should only do it when | is specified in core pattern.
Otherwise we would
have core file as
core.my exectutable
Martin and I have already gone over this in a previous post and I think we agree
that this is enough of a corner case, and so easily worked around, that we can
address it in a later patch.
Neil
-aneesh
--
/***
*Neil Horman
*Software Engineer
*Red
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Sun, Jul 29, 2007 at 11:34:18AM +0200, Martin Pitt wrote:
Hi Neil,
Neil Horman [2007-07-28 13:21 -0400]:
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.
Oh, handling escaping and quoting is going to make it fairly
complicated, but sure, if you need that for other things, too, that
would solve the remaining case. I just wonder if, instead of
implementing escaping, it wouldn't be easier to first split on spaces
and then escape macros?
If it were just this case, then yes. But we also need to make this work
properly for when core_pattern is not a pipe. So the macro expansion routine
needs to be generalized somewhat. Certainly doable, it just takes a bit more
testing and care in coding. I'll start looking into it in the upcomming week.
Thank you for clearing me up on this. So it would seem we're ok with what
we
have now, correct?
Absolutely, yes.
We just have a potential corner case to address, which I can
reasonably handle with a modification to split_argv, that I have a
todo on next week.
Right, it's really just for perfectionism. Spaces in executable names
are EBW anyway, and readlink()ing /proc/pid/exe is much more robust
anyway in terms of a small and orthogonal interface.
Understood.
If the upstream kernel guys don't worry about it and consider it a
blocker for merging, I don't either. :-)
Copy that
Thanks Regards
Neil
Thanks a lot,
Martin
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
--
/***
*Neil Horman
*Software Engineer
*Red Hat, Inc.
[EMAIL PROTECTED]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***/
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
Neil Horman wrote:
[...]
+ /* core limit size */
+ case 'c':
+ rc = snprintf(out_ptr, out_end - out_ptr,
+ %lu,
current-signal-rlim[RLIMIT_CORE].rlim_cur);
Trailing space.
[...]
- if(call_usermodehelper_pipe(corename+1, NULL, NULL, file)) {
+ if(call_usermodehelper_pipe(corename+1, helper_argv, NULL,
file)) {
^
Use tabs, and a missing space after '('.
Eugene
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Sun, Jul 29, 2007 at 09:03:29PM +0800, Eugene Teo wrote:
Neil Horman wrote:
[...]
+ /* core limit size */
+ case 'c':
+ rc = snprintf(out_ptr, out_end - out_ptr,
+ %lu,
current-signal-rlim[RLIMIT_CORE].rlim_cur);
Trailing space.
[...]
- if(call_usermodehelper_pipe(corename+1, NULL, NULL, file)) {
+ if(call_usermodehelper_pipe(corename+1, helper_argv, NULL,
file)) {
^
Use tabs, and a missing space after '('.
I think your reader is screwing up the patch. The version that I sent, which I
have here, shows that as all tabs, no spaces.
As for the '(', yeah, that looks like its been there for awhile. I'll clean it
all up when I address macro expansion, as per the conversation Martin and I have
been holding.
Regards
Neil
Eugene
--
/***
*Neil Horman
*Software Engineer
*Red Hat, Inc.
[EMAIL PROTECTED]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***/
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Sat, Jul 28, 2007 at 03:52:02PM -0700, Jeremy Fitzhardinge wrote:
> Neil Horman wrote:
> > 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.
> >
>
> No, please don't. My original argv_split did that, and it was just way
> too complex. If you need complex quoting, you can always point it at a
> shell script and handle it there.
>
> J
Ok, well then, it seems this corner case is much too harry to just fix up
immediately. Given that we certainly don't handle quoted strings now, and the
fact that this is a case that will almost never come up, and can be esaily
worked around, lets address it at some time after we get this base functionality
in place
Regards
Neil
--
/***
*Neil Horman
*Software Engineer
*Red Hat, Inc.
[EMAIL PROTECTED]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
Neil Horman wrote:
> 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.
>
No, please don't. My original argv_split did that, and it was just way
too complex. If you need complex quoting, you can always point it at a
shell script and handle it there.
J
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Sat, Jul 28, 2007 at 06:17:25PM +0200, Martin Pitt wrote:
> 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.
>
Ahh, well then you should have nothing to worry about, this patch expands them
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.
> > > 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// (and that's
> indeed what apport does to figure out relevant parts from the
> environment like the locale).
>
Agreed, /proc//* will be available while the helper app
runs.
> 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:
>
I think you're correct, I misundersood you previously. Apologies.
> 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).
>
Yes, that is exactly correct.
> 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'].
>
Also correct. Thats a pretty big corner case, and I personally don't think an
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.
> 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.
>
Ah, my bad. We're on the same page then, and I just misunderstood what you were
referring to when you said macro expansion. I thought you wanted to have
core_pattern translate things like $HOME and soforth, which can more esaily be
passed to things like apport via environment inheritence, which we can look at
at a later date.
> I just wanted to mention this little
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// (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 Pitthttp://www.piware.de Ubuntu Developer http://www.ubuntu.com Debian Developer http://www.debian.org signature.asc Description: Digital signature
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Sat, Jul 28, 2007 at 11:23:55AM +0200, Martin Pitt wrote:
> Hi Neil,
>
> thanks a lot for your work on this!
>
> Neil Horman [2007-07-27 16:08 -0400]:
> > Hey
> > Patch 2/3 of my core_pattern enhancements. This patch is a rewrite of
> > my previous post for this enhancement. It uses jeremy's
> > split_argv/free_argv
> > library functions to translate core_pattern into an argv array to be passed
> > to
> > the user mode helper process.
> > [...]
> > if (ispipe) {
> > core_limit = RLIM_INFINITY;
> > + helper_argv = argv_split(GFP_KERNEL, corename+1, _argc);
>
> 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.
> 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 would love to use macros instead since it looks a bit cleaner, and
> personally I do not care about the "executable name" macro for Apport
> [1] (I grab it from /proc/pid/exe), but I wanted to mention this
> possible caveat before it goes upstream.
>
If you don't want to use a given macro, fine, don't use it. If you want to use
a macro, thats fine too, but I really don't want to offer the ability to pass
macro's in core_pattern (it really just makes the parsing too hairy and prone to
error). If you want to pass macros to apport, or whatever, we'll just let the
user mode helper inherit the crashing processes environment. Its much easier
work, just as usefull, and more importantly, can be done later as a separate
piece of work.
Regards
Neil
> Thank you,
>
> Martin
>
> [1] https://wiki.ubuntu.com/Apport
>
> --
> Martin Pitthttp://www.piware.de
> Ubuntu Developer http://www.ubuntu.com
> Debian Developer http://www.debian.org
--
/***
*Neil Horman
*Software Engineer
*Red Hat, Inc.
[EMAIL PROTECTED]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
Hi Neil,
thanks a lot for your work on this!
Neil Horman [2007-07-27 16:08 -0400]:
> Hey
> Patch 2/3 of my core_pattern enhancements. This patch is a rewrite of
> my previous post for this enhancement. It uses jeremy's split_argv/free_argv
> library functions to translate core_pattern into an argv array to be passed to
> the user mode helper process.
> [...]
> if (ispipe) {
> core_limit = RLIM_INFINITY;
> + helper_argv = argv_split(GFP_KERNEL, corename+1, _argc);
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.
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 would love to use macros instead since it looks a bit cleaner, and
personally I do not care about the "executable name" macro for Apport
[1] (I grab it from /proc/pid/exe), but I wanted to mention this
possible caveat before it goes upstream.
Thank you,
Martin
[1] https://wiki.ubuntu.com/Apport
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
signature.asc
Description: Digital signature
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
Hi Neil,
thanks a lot for your work on this!
Neil Horman [2007-07-27 16:08 -0400]:
Hey
Patch 2/3 of my core_pattern enhancements. This patch is a rewrite of
my previous post for this enhancement. It uses jeremy's split_argv/free_argv
library functions to translate core_pattern into an argv array to be passed to
the user mode helper process.
[...]
if (ispipe) {
core_limit = RLIM_INFINITY;
+ helper_argv = argv_split(GFP_KERNEL, corename+1, helper_argc);
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.
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 would love to use macros instead since it looks a bit cleaner, and
personally I do not care about the executable name macro for Apport
[1] (I grab it from /proc/pid/exe), but I wanted to mention this
possible caveat before it goes upstream.
Thank you,
Martin
[1] https://wiki.ubuntu.com/Apport
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
signature.asc
Description: Digital signature
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Sat, Jul 28, 2007 at 11:23:55AM +0200, Martin Pitt wrote:
Hi Neil,
thanks a lot for your work on this!
Neil Horman [2007-07-27 16:08 -0400]:
Hey
Patch 2/3 of my core_pattern enhancements. This patch is a rewrite of
my previous post for this enhancement. It uses jeremy's
split_argv/free_argv
library functions to translate core_pattern into an argv array to be passed
to
the user mode helper process.
[...]
if (ispipe) {
core_limit = RLIM_INFINITY;
+ helper_argv = argv_split(GFP_KERNEL, corename+1, helper_argc);
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.
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 would love to use macros instead since it looks a bit cleaner, and
personally I do not care about the executable name macro for Apport
[1] (I grab it from /proc/pid/exe), but I wanted to mention this
possible caveat before it goes upstream.
If you don't want to use a given macro, fine, don't use it. If you want to use
a macro, thats fine too, but I really don't want to offer the ability to pass
macro's in core_pattern (it really just makes the parsing too hairy and prone to
error). If you want to pass macros to apport, or whatever, we'll just let the
user mode helper inherit the crashing processes environment. Its much easier
work, just as usefull, and more importantly, can be done later as a separate
piece of work.
Regards
Neil
Thank you,
Martin
[1] https://wiki.ubuntu.com/Apport
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
--
/***
*Neil Horman
*Software Engineer
*Red Hat, Inc.
[EMAIL PROTECTED]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***/
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 Pitthttp://www.piware.de Ubuntu Developer http://www.ubuntu.com Debian Developer http://www.debian.org signature.asc Description: Digital signature
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Sat, Jul 28, 2007 at 06:17:25PM +0200, Martin Pitt wrote:
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.
Ahh, well then you should have nothing to worry about, this patch expands them
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.
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).
Agreed, /proc/pid of crashing process/* will be available while the helper app
runs.
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:
I think you're correct, I misundersood you previously. Apologies.
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).
Yes, that is exactly correct.
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'].
Also correct. Thats a pretty big corner case, and I personally don't think an
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.
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.
Ah, my bad. We're on the same page then, and I just misunderstood what you were
referring to when you said macro expansion. I thought you wanted to have
core_pattern translate things like $HOME and soforth, which can more esaily be
passed to things like apport via environment inheritence, which we can look at
at a later date.
I just wanted to mention this little problem for the sake of
correctness.
Thank you, and have a nice weekend!
Martin
Thank
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
Neil Horman wrote:
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.
No, please don't. My original argv_split did that, and it was just way
too complex. If you need complex quoting, you can always point it at a
shell script and handle it there.
J
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Sat, Jul 28, 2007 at 03:52:02PM -0700, Jeremy Fitzhardinge wrote:
Neil Horman wrote:
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.
No, please don't. My original argv_split did that, and it was just way
too complex. If you need complex quoting, you can always point it at a
shell script and handle it there.
J
Ok, well then, it seems this corner case is much too harry to just fix up
immediately. Given that we certainly don't handle quoted strings now, and the
fact that this is a case that will almost never come up, and can be esaily
worked around, lets address it at some time after we get this base functionality
in place
Regards
Neil
--
/***
*Neil Horman
*Software Engineer
*Red Hat, Inc.
[EMAIL PROTECTED]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***/
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Fri, Jul 27, 2007 at 01:54:19PM -0700, Jeremy Fitzhardinge wrote: > Neil Horman wrote: > > + int helper_argc = 0; > > > > + helper_argv = argv_split(GFP_KERNEL, corename+1, _argc); > > > > Hm, I suspect most users of argv_split don't really care about argc, so > it would useful to change argv_split to take NULL as the argc pointer, > rather than declare a bunch of unused variables. Interested in throwing > a patch together? > > J Gladly, I'll take care of it next week. Regards Neil - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
Neil Horman wrote: > + int helper_argc = 0; > > + helper_argv = argv_split(GFP_KERNEL, corename+1, _argc); > Hm, I suspect most users of argv_split don't really care about argc, so it would useful to change argv_split to take NULL as the argc pointer, rather than declare a bunch of unused variables. Interested in throwing a patch together? J - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
Hey
Patch 2/3 of my core_pattern enhancements. This patch is a rewrite of
my previous post for this enhancement. It uses jeremy's split_argv/free_argv
library functions to translate core_pattern into an argv array to be passed to
the user mode helper process. It also adds a translation to format_corename
such that the origional value of RLIMIT_CORE can be passed to userspace as an
argument.
Thanks & Regards
Neil
Signed-off-by: Neil Horman <[EMAIL PROTECTED]>
exec.c | 25 ++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index c0b5def..6855a52 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -51,6 +51,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1543,6 +1544,14 @@ static int format_corename(char *corename, const char
*pattern, long signr)
goto out;
out_ptr += rc;
break;
+ /* core limit size */
+ case 'c':
+ rc = snprintf(out_ptr, out_end - out_ptr,
+ "%lu",
current->signal->rlim[RLIMIT_CORE].rlim_cur);
+ if (rc > out_end - out_ptr)
+ goto out;
+ out_ptr += rc;
+ break;
default:
break;
}
@@ -1727,6 +1736,9 @@ int do_coredump(long signr, int exit_code, struct pt_regs
* regs)
int flag = 0;
int ispipe = 0;
unsigned long core_limit = current->signal->rlim[RLIMIT_CORE].rlim_cur;
+ char **helper_argv = NULL;
+ int helper_argc = 0;
+ char *delimit;
audit_core_dumps(signr);
@@ -1775,14 +1787,18 @@ int do_coredump(long signr, int exit_code, struct
pt_regs * regs)
* at which point file size limits and permissions will be imposed
* as it does with any other process
*/
- if ((!ispipe) &&
- (core_limit < binfmt->min_coredump))
+ if ((!ispipe) && (core_limit < binfmt->min_coredump))
goto fail_unlock;
if (ispipe) {
core_limit = RLIM_INFINITY;
+ helper_argv = argv_split(GFP_KERNEL, corename+1, _argc);
+ /* Terminate the string before the first option */
+ delimit = strchr(corename, ' ');
+ if (delimit)
+ *delimit = '\0';
/* SIGPIPE can happen, but it's just never processed */
- if(call_usermodehelper_pipe(corename+1, NULL, NULL, )) {
+ if(call_usermodehelper_pipe(corename+1, helper_argv, NULL,
)) {
printk(KERN_INFO "Core dump to %s pipe failed\n",
corename);
goto fail_unlock;
@@ -1817,6 +1833,9 @@ int do_coredump(long signr, int exit_code, struct pt_regs
* regs)
close_fail:
filp_close(file, NULL);
fail_unlock:
+ if (helper_argv)
+ argv_free(helper_argv);
+
current->fsuid = fsuid;
complete_all(>core_done);
fail:
--
/***
*Neil Horman
*Software Engineer
*Red Hat, Inc.
[EMAIL PROTECTED]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
Hey
Patch 2/3 of my core_pattern enhancements. This patch is a rewrite of
my previous post for this enhancement. It uses jeremy's split_argv/free_argv
library functions to translate core_pattern into an argv array to be passed to
the user mode helper process. It also adds a translation to format_corename
such that the origional value of RLIMIT_CORE can be passed to userspace as an
argument.
Thanks Regards
Neil
Signed-off-by: Neil Horman [EMAIL PROTECTED]
exec.c | 25 ++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index c0b5def..6855a52 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -51,6 +51,7 @@
#include linux/cn_proc.h
#include linux/audit.h
#include linux/signalfd.h
+#include linux/string.h
#include asm/uaccess.h
#include asm/mmu_context.h
@@ -1543,6 +1544,14 @@ static int format_corename(char *corename, const char
*pattern, long signr)
goto out;
out_ptr += rc;
break;
+ /* core limit size */
+ case 'c':
+ rc = snprintf(out_ptr, out_end - out_ptr,
+ %lu,
current-signal-rlim[RLIMIT_CORE].rlim_cur);
+ if (rc out_end - out_ptr)
+ goto out;
+ out_ptr += rc;
+ break;
default:
break;
}
@@ -1727,6 +1736,9 @@ int do_coredump(long signr, int exit_code, struct pt_regs
* regs)
int flag = 0;
int ispipe = 0;
unsigned long core_limit = current-signal-rlim[RLIMIT_CORE].rlim_cur;
+ char **helper_argv = NULL;
+ int helper_argc = 0;
+ char *delimit;
audit_core_dumps(signr);
@@ -1775,14 +1787,18 @@ int do_coredump(long signr, int exit_code, struct
pt_regs * regs)
* at which point file size limits and permissions will be imposed
* as it does with any other process
*/
- if ((!ispipe)
- (core_limit binfmt-min_coredump))
+ if ((!ispipe) (core_limit binfmt-min_coredump))
goto fail_unlock;
if (ispipe) {
core_limit = RLIM_INFINITY;
+ helper_argv = argv_split(GFP_KERNEL, corename+1, helper_argc);
+ /* Terminate the string before the first option */
+ delimit = strchr(corename, ' ');
+ if (delimit)
+ *delimit = '\0';
/* SIGPIPE can happen, but it's just never processed */
- if(call_usermodehelper_pipe(corename+1, NULL, NULL, file)) {
+ if(call_usermodehelper_pipe(corename+1, helper_argv, NULL,
file)) {
printk(KERN_INFO Core dump to %s pipe failed\n,
corename);
goto fail_unlock;
@@ -1817,6 +1833,9 @@ int do_coredump(long signr, int exit_code, struct pt_regs
* regs)
close_fail:
filp_close(file, NULL);
fail_unlock:
+ if (helper_argv)
+ argv_free(helper_argv);
+
current-fsuid = fsuid;
complete_all(mm-core_done);
fail:
--
/***
*Neil Horman
*Software Engineer
*Red Hat, Inc.
[EMAIL PROTECTED]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***/
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
Neil Horman wrote: + int helper_argc = 0; + helper_argv = argv_split(GFP_KERNEL, corename+1, helper_argc); Hm, I suspect most users of argv_split don't really care about argc, so it would useful to change argv_split to take NULL as the argc pointer, rather than declare a bunch of unused variables. Interested in throwing a patch together? J - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe
On Fri, Jul 27, 2007 at 01:54:19PM -0700, Jeremy Fitzhardinge wrote: Neil Horman wrote: + int helper_argc = 0; + helper_argv = argv_split(GFP_KERNEL, corename+1, helper_argc); Hm, I suspect most users of argv_split don't really care about argc, so it would useful to change argv_split to take NULL as the argc pointer, rather than declare a bunch of unused variables. Interested in throwing a patch together? J Gladly, I'll take care of it next week. Regards Neil - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
