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/
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/