You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Jeff Trawick <tr...@attglobal.net> on 2003/02/05 15:59:17 UTC
[PATCH] letting the app do something useful when apr_proc_create()
fails in the child process
On Unix, some failures of apr_proc_create() are not noticed in the
calling process and so apr_proc_create() returns APR_SUCCESS even though
it failed.
Some of the potential failures could be discovered in the parent by
using extra syscalls (e.g., use stat to make sure the program actually
exists and can be executed by this user, use stat to make sure the
desired working directory actually exists and can be chdir-ed to by this
user). It isn't appropriate to burn the required cycles by default, but
it would be nice to allow the app to turn on this extra checking, since
failures of apr_proc_create() can be handled much cleaner if the
function returns an error status.
Alternatively, APR could allow the application to get called in the
child process in the failure cases and allow it to do whatever is
appropriate (log a message, synchronize with the parent process, etc.).
I think both features are important. Using Apache as an example, I
would imagine that for situations where apr_proc_create() is not called
frequently (e.g., piped log program), it would be useful to turn on any
available extra checking in the parent so that the best possible
diagnostics can be given to the admin with minimal extra coding in
Apache. Such extra overhead is of no concern for a path that runs so
infrequently. On the other hand, when running CGIs (something the
server may do very frequently), any extra overhead would be
inappropriate given that we expect it to work in general.
This particular patch allows the latter approach -- let the app get
involved when apr_proc_create() fails in the child. This is useful with
or without the ability to try to find potential errors in the parent
since not everything can be checked.
Any concerns, particularly with respect to how the app determines if the
feature is available? I think it would be fine to make the feature
always available but to simply note that on some platforms the
application-specified error function would never be called.
Re: [PATCH] letting the app do something useful when apr_proc_create()
fails in the child process
Posted by Jeff Trawick <tr...@attglobal.net>.
Bill Stoddard wrote:
> +1, this looks like very useful function. I would like to see a bit
> more explanation in the child_errfn_set making it clear that this
> function is used to accurately report failures in the 'exec' of a 'fork
> & exec'. Also explicitly state that this function can only be used on
> systems that use fork in apr_proc_create() (yea, it's obvious if you
> understand what the function is doing...)
will do...
Thanks,
Jeff
Re: [PATCH] letting the app do something useful when apr_proc_create()
fails in the child process
Posted by Bill Stoddard <bi...@wstoddard.com>.
+1, this looks like very useful function. I would like to see a bit
more explanation in the child_errfn_set making it clear that this
function is used to accurately report failures in the 'exec' of a 'fork
& exec'. Also explicitly state that this function can only be used on
systems that use fork in apr_proc_create() (yea, it's obvious if you
understand what the function is doing...)
Bill
Jeff Trawick wrote:
> On Unix, some failures of apr_proc_create() are not noticed in the
> calling process and so apr_proc_create() returns APR_SUCCESS even though
> it failed.
>
> Some of the potential failures could be discovered in the parent by
> using extra syscalls (e.g., use stat to make sure the program actually
> exists and can be executed by this user, use stat to make sure the
> desired working directory actually exists and can be chdir-ed to by this
> user). It isn't appropriate to burn the required cycles by default, but
> it would be nice to allow the app to turn on this extra checking, since
> failures of apr_proc_create() can be handled much cleaner if the
> function returns an error status.
>
> Alternatively, APR could allow the application to get called in the
> child process in the failure cases and allow it to do whatever is
> appropriate (log a message, synchronize with the parent process, etc.).
>
> I think both features are important. Using Apache as an example, I
> would imagine that for situations where apr_proc_create() is not called
> frequently (e.g., piped log program), it would be useful to turn on any
> available extra checking in the parent so that the best possible
> diagnostics can be given to the admin with minimal extra coding in
> Apache. Such extra overhead is of no concern for a path that runs so
> infrequently. On the other hand, when running CGIs (something the
> server may do very frequently), any extra overhead would be
> inappropriate given that we expect it to work in general.
>
> This particular patch allows the latter approach -- let the app get
> involved when apr_proc_create() fails in the child. This is useful with
> or without the ability to try to find potential errors in the parent
> since not everything can be checked.
>
> Any concerns, particularly with respect to how the app determines if the
> feature is available? I think it would be fine to make the feature
> always available but to simply note that on some platforms the
> application-specified error function would never be called.
>
>
> ------------------------------------------------------------------------
>
> Index: include/apr.h.in
> ===================================================================
> RCS file: /home/cvs/apr/include/apr.h.in,v
> retrieving revision 1.118
> diff -u -r1.118 apr.h.in
> --- include/apr.h.in 22 Jan 2003 18:25:59 -0000 1.118
> +++ include/apr.h.in 5 Feb 2003 14:40:52 -0000
> @@ -139,6 +139,7 @@
> #define APR_HAS_SENDFILE @sendfile@
> #define APR_HAS_MMAP @mmap@
> #define APR_HAS_FORK @fork@
> +#define APR_HAS_CHILD_ERRFN @fork@
> #define APR_HAS_RANDOM @rand@
> #define APR_HAS_OTHER_CHILD @oc@
> #define APR_HAS_DSO @aprdso@
> Index: include/apr.hnw
> ===================================================================
> RCS file: /home/cvs/apr/include/apr.hnw,v
> retrieving revision 1.28
> diff -u -r1.28 apr.hnw
> --- include/apr.hnw 27 Jan 2003 17:09:09 -0000 1.28
> +++ include/apr.hnw 5 Feb 2003 14:40:52 -0000
> @@ -207,6 +207,7 @@
> #define APR_HAS_SENDFILE 0
> #define APR_HAS_MMAP 0
> #define APR_HAS_FORK 0
> +#define APR_HAS_CHILD_ERRFN 0
> #define APR_HAS_RANDOM 1
> #define APR_HAS_OTHER_CHILD 0
> #define APR_HAS_DSO 1
> Index: include/apr.hw
> ===================================================================
> RCS file: /home/cvs/apr/include/apr.hw,v
> retrieving revision 1.107
> diff -u -r1.107 apr.hw
> --- include/apr.hw 28 Jan 2003 21:11:22 -0000 1.107
> +++ include/apr.hw 5 Feb 2003 14:40:52 -0000
> @@ -280,6 +280,7 @@
> #define APR_HAS_THREADS 1
> #define APR_HAS_MMAP 1
> #define APR_HAS_FORK 0
> +#define APR_HAS_CHILD_ERRFN 0
> #define APR_HAS_RANDOM 1
> #define APR_HAS_OTHER_CHILD 0
> #define APR_HAS_DSO 1
> Index: include/apr_thread_proc.h
> ===================================================================
> RCS file: /home/cvs/apr/include/apr_thread_proc.h,v
> retrieving revision 1.90
> diff -u -r1.90 apr_thread_proc.h
> --- include/apr_thread_proc.h 4 Feb 2003 20:10:34 -0000 1.90
> +++ include/apr_thread_proc.h 5 Feb 2003 14:40:52 -0000
> @@ -177,6 +177,20 @@
> #endif
> };
>
> +#if APR_HAS_CHILD_ERRFN
> +/**
> + * The prototype for APR child errfn functions. It is passed the
> + * following information:
> + * @param proc Representation of proc that couldn't be created
> + * @param err APR error code describing the error
> + * @param description Text description of type of processing which failed
> + * @param userdata Value specified by application on call to
> + * apr_procattr_child_errfn_set()
> + */
> +typedef void (apr_child_errfn_t)(apr_proc_t *proc, apr_status_t err,
> + const char *description, void *userdata);
> +#endif
> +
> /** Opaque Thread structure. */
> typedef struct apr_thread_t apr_thread_t;
> /** Opaque Thread attributes structure. */
> @@ -485,6 +499,21 @@
> APR_DECLARE(apr_status_t) apr_procattr_limit_set(apr_procattr_t *attr,
> apr_int32_t what,
> struct rlimit *limit);
> +#endif
> +
> +#if APR_HAS_CHILD_ERRFN
> +/**
> + * Specify an error function to be called in the child process if APR
> + * encounters an error in the child prior to running the specified program.
> + * @param child_errfn The function to call in the child process.
> + * @param userdata Parameter to be passed to errfn.
> + * @remark At the present time, it is only useful and only implemented
> + * on platforms that have fork(). Programs should reference the feature
> + * test macro APR_HAS_CHILD_ERRFN.
> + */
> +APR_DECLARE(apr_status_t) apr_procattr_child_errfn_set(apr_procattr_t *attr,
> + apr_child_errfn_t *errfn,
> + void *userdata);
> #endif
>
> #if APR_HAS_FORK
> Index: include/arch/unix/apr_arch_threadproc.h
> ===================================================================
> RCS file: /home/cvs/apr/include/arch/unix/apr_arch_threadproc.h,v
> retrieving revision 1.3
> diff -u -r1.3 apr_arch_threadproc.h
> --- include/arch/unix/apr_arch_threadproc.h 7 Jan 2003 00:52:54 -0000 1.3
> +++ include/arch/unix/apr_arch_threadproc.h 5 Feb 2003 14:40:52 -0000
> @@ -134,6 +134,8 @@
> #ifdef RLIMIT_NOFILE
> struct rlimit *limit_nofile;
> #endif
> + apr_child_errfn_t *errfn;
> + void *errfn_userdata;
> };
>
> #endif /* ! THREAD_PROC_H */
> Index: threadproc/unix/proc.c
> ===================================================================
> RCS file: /home/cvs/apr/threadproc/unix/proc.c,v
> retrieving revision 1.63
> diff -u -r1.63 proc.c
> --- threadproc/unix/proc.c 4 Feb 2003 20:12:29 -0000 1.63
> +++ threadproc/unix/proc.c 5 Feb 2003 14:40:52 -0000
> @@ -295,6 +295,15 @@
> return APR_SUCCESS;
> }
>
> +APR_DECLARE(apr_status_t) apr_procattr_child_errfn_set(apr_procattr_t *attr,
> + apr_child_errfn_t *errfn,
> + void *userdata)
> +{
> + attr->errfn = errfn;
> + attr->errfn_userdata = userdata;
> + return APR_SUCCESS;
> +}
> +
> APR_DECLARE(apr_status_t) apr_proc_create(apr_proc_t *new,
> const char *progname,
> const char * const *args,
> @@ -368,11 +377,19 @@
>
> if (attr->currdir != NULL) {
> if (chdir(attr->currdir) == -1) {
> + if (attr->errfn) {
> + attr->errfn(new, errno, "change of working directory failed",
> + attr->errfn_userdata);
> + }
> exit(-1); /* We have big problems, the child should exit. */
> }
> }
>
> if ((status = limit_proc(attr)) != APR_SUCCESS) {
> + if (attr->errfn) {
> + attr->errfn(new, errno, "setting of resource limits failed",
> + attr->errfn_userdata);
> + }
> exit(-1); /* We have big problems, the child should exit. */
> }
>
> @@ -422,6 +439,14 @@
>
> execvp(progname, (char * const *)args);
> }
> + if (attr->errfn) {
> + char *desc;
> +
> + desc = apr_psprintf(pool, "exec of '%s' failed",
> + progname);
> + attr->errfn(new, errno, desc, attr->errfn_userdata);
> + }
> +
> exit(-1); /* if we get here, there is a problem, so exit with an
> * error code. */
> }
Re: [PATCH] letting the app do something useful when apr_proc_create()
fails in the child process
Posted by Jeff Trawick <tr...@attglobal.net>.
Jeff Trawick wrote:
> Index: include/apr.h.in
> ===================================================================
> RCS file: /home/cvs/apr/include/apr.h.in,v
> retrieving revision 1.118
> diff -u -r1.118 apr.h.in
> --- include/apr.h.in 22 Jan 2003 18:25:59 -0000 1.118
> +++ include/apr.h.in 5 Feb 2003 14:40:52 -0000
> @@ -139,6 +139,7 @@
> #define APR_HAS_SENDFILE @sendfile@
> #define APR_HAS_MMAP @mmap@
> #define APR_HAS_FORK @fork@
> +#define APR_HAS_CHILD_ERRFN @fork@
Thinking ahead, maybe this should be more general, and should be some
concise name that means that apr_proc_create() *may* use fork().
Two other things I'm thinking of:
. adding a separate attribute that tells apr_proc_create() to burn
cycles prior to fork() to look for potential errors that would be
encountered in the child (e.g., make sure the executable exists and can
be executed, make sure any specified curdir exists)
we would want one feature macro that covers the function of my patch as
well as the burn-extra-cycles option
. at least one platform where we currently use fork+exec has more
efficient spawn-type function... if we determined at run-time that we
could do everything the app wanted via spawn, we'd be bypassing the code
that made use of these new proc attributes
hence the phrase "*may* use fork()"
Re: [PATCH] letting the app do something useful when apr_proc_create()
fails in the child process
Posted by Jeff Trawick <tr...@attglobal.net>.
Greg Ames wrote:
> > Alternatively, APR could allow the application to get called in the
> > child process in the failure cases and allow it to do whatever is
> > appropriate (log a message, synchronize with the parent process, etc.).
>
>
> Couldn't the stat's, chdir's, etc. be done only after a failure to keep
> the overhead out of the mainline path? Then each app wouldn't have to
> rewrite the basic diagnostic code.
After a failure of exec() or chdir() we already know the reason of the
error, so such extra checking in the error path is not helpful. The
problem with waiting until after something fails in the child is that
there is no straightforward way for APR to communicate the specific
reason for failure to the parent. The parent (the app that calls
apr_proc_create()) is long gone from its call to apr_proc_create(). All
that happens is that the child exits with code -1, and the parent can
find out by calling apr_proc_wait().
A particular application may have a suitable way to communicate this
info to the parent in its own code, and APR will make that info
available to the application via the child error function.
Re: [PATCH] letting the app do something useful when apr_proc_create()
fails in the child process
Posted by Jeff Trawick <tr...@attglobal.net>.
Greg Ames wrote:
> > Alternatively, APR could allow the application to get called in the
> > child process in the failure cases and allow it to do whatever is
> > appropriate (log a message, synchronize with the parent process, etc.).
>
>
> Couldn't the stat's, chdir's, etc. be done only after a failure to keep
> the overhead out of the mainline path? Then each app wouldn't have to
> rewrite the basic diagnostic code.
After a failure of exec() or chdir() we already know the reason of the
error, so such extra checking in the error path is not helpful. The
problem with waiting until after something fails in the child is that
there is no straightforward way for APR to communicate the specific
reason for failure to the parent. The parent (the app that calls
apr_proc_create()) is long gone from its call to apr_proc_create(). All
that happens is that the child exits with code -1, and the parent can
find out by calling apr_proc_wait().
A particular application may have a suitable way to communicate this
info to the parent in its own code, and APR will make that info
available to the application via the child error function.
Re: [PATCH] letting the app do something useful when apr_proc_create()
fails in the child process
Posted by Greg Ames <gr...@apache.org>.
Jeff Trawick wrote:
> On Unix, some failures of apr_proc_create() are not noticed in the
> calling process and so apr_proc_create() returns APR_SUCCESS even though
> it failed.
++1 in concept. It has bugged me for a long time that httpd 2.0 only logs
"Premature end of script headers" no matter what is wrong with a CGI. ISTR that
1.3 provided better diagnostic info, perhaps at the cost of more overhead.
> Some of the potential failures could be discovered in the parent by
> using extra syscalls (e.g., use stat to make sure the program actually
> exists and can be executed by this user, use stat to make sure the
> desired working directory actually exists and can be chdir-ed to by this
> user). It isn't appropriate to burn the required cycles by default, but
> it would be nice to allow the app to turn on this extra checking, since
> failures of apr_proc_create() can be handled much cleaner if the
> function returns an error status.
>
> Alternatively, APR could allow the application to get called in the
> child process in the failure cases and allow it to do whatever is
> appropriate (log a message, synchronize with the parent process, etc.).
Couldn't the stat's, chdir's, etc. be done only after a failure to keep the
overhead out of the mainline path? Then each app wouldn't have to rewrite the
basic diagnostic code.
Having an optional callback in the app do the logging is a good idea. It gets
around the problem of the library spewing messages to stderr which the app may
not want.
Greg
Re: [PATCH] letting the app do something useful when
apr_proc_create() fails in the child process
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Wednesday, February 5, 2003 9:59 AM -0500 Jeff Trawick
<tr...@attglobal.net> wrote:
> Any concerns, particularly with respect to how the app determines
> if the feature is available? I think it would be fine to make the
> feature always available but to simply note that on some platforms
> the application-specified error function would never be called.
Sure. I'm not of the type that thinks that everything must work
identically on all platforms. =)
Note, I don't think this patch would compile when APR_HAS_CHILD_ERRFN
is 0. The apr_child_errfn_t wouldn't be defined, but it'd be
references in apr_arch_threadproc.h. Not that I'm aware of any Unix
platforms without fork(), but... -- justin
Re: [PATCH] letting the app do something useful when apr_proc_create()
fails in the child process
Posted by Greg Ames <gr...@apache.org>.
Jeff Trawick wrote:
> On Unix, some failures of apr_proc_create() are not noticed in the
> calling process and so apr_proc_create() returns APR_SUCCESS even though
> it failed.
++1 in concept. It has bugged me for a long time that httpd 2.0 only logs
"Premature end of script headers" no matter what is wrong with a CGI. ISTR that
1.3 provided better diagnostic info, perhaps at the cost of more overhead.
> Some of the potential failures could be discovered in the parent by
> using extra syscalls (e.g., use stat to make sure the program actually
> exists and can be executed by this user, use stat to make sure the
> desired working directory actually exists and can be chdir-ed to by this
> user). It isn't appropriate to burn the required cycles by default, but
> it would be nice to allow the app to turn on this extra checking, since
> failures of apr_proc_create() can be handled much cleaner if the
> function returns an error status.
>
> Alternatively, APR could allow the application to get called in the
> child process in the failure cases and allow it to do whatever is
> appropriate (log a message, synchronize with the parent process, etc.).
Couldn't the stat's, chdir's, etc. be done only after a failure to keep the
overhead out of the mainline path? Then each app wouldn't have to rewrite the
basic diagnostic code.
Having an optional callback in the app do the logging is a good idea. It gets
around the problem of the library spewing messages to stderr which the app may
not want.
Greg
Re: APR mem leak?
Posted by Cliff Woolley <jw...@virginia.edu>.
On Fri, 14 Feb 2003, Dagfinn Aarvaag wrote:
> When creating TCP connections the client leaks memory, 8/16 kbyte, for
> every connection that is created. The server side does not leak.
Which client?? Which server?
--Cliff
RE: APR mem leak?
Posted by Sander Striker <st...@apache.org>.
> From: Dagfinn Aarvaag [mailto:dagfinn.aarvaag@beepscience.com]
> Sent: Friday, February 14, 2003 4:42 PM
> Hi,
>
> I want to report a possible bug in the APR that I have seen for a while.
> May be someone familiar with the code can find it easy?
>
> When creating TCP connections the client leaks memory, 8/16 kbyte, for every
> connection that is created.
> The server side does not leak.
'The client', 'The server'?
Something tells me this is not a leak. You are just seeing the default
size of a pool chunk being allocated (8k).
Sander
APR mem leak?
Posted by Dagfinn Aarvaag <da...@beepscience.com>.
Hi,
I want to report a possible bug in the APR that I have seen for a while.
May be someone familiar with the code can find it easy?
When creating TCP connections the client leaks memory, 8/16 kbyte, for every
connection that is created.
The server side does not leak.
/Dagfinn
Re: [PATCH] letting the app do something useful when
apr_proc_create() fails in the child process
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 02:19 PM 2/5/2003, Jeff Trawick wrote:
>William A. Rowe, Jr. wrote:
>
>>At 11:53 AM 2/5/2003, Jeff Trawick wrote:
>>
>>>on APR not providing a string which tells what type of processing failed:
>>>
>>>With no string from APR, you don't know if, for example, the failure was EPERM because
>>>
>>>a) permissions on working directory were bad
>>>b) permissions on executable were bad
>>>c) no permission to raise rlimits to specified value
>>>
>>>A certain large class of users would really benefit from such information, even if it is not in their native language and they have to feed it into google. (But surely if we'd eventually translate other APR strings then an infrastructure would be in place.)
>>
>>
>>Oh, I agree there. I just wouldn't stuff the program name and do
>>the string munging, let the user format it all. What about simply
>>passing a status flag so the callback can actually react to those
>>different cases (e.g. APR_PROC_FAIL_CWD, APR_PROC_FAIL_LOAD,
>>APR_PROC_FAIL_PERMS etc)?
>
>I did consider different enums to cover different processing steps that might fail but I'd then want APR to give me a stringify function so I didn't have to check the enums myself and potentially have to make my program smarter when APR added a new potential failure point. Too complicating.
Can we create a flavor of apr_errorstring for this purpose, then? Pass
the apr_errorstring_proccreate() fn the apr_procattr_t, the apr_status_t
and the reason, and get back a thorough explanation string of the failure?
Bill
Re: [PATCH] letting the app do something useful when apr_proc_create()
fails in the child process
Posted by Jeff Trawick <tr...@attglobal.net>.
William A. Rowe, Jr. wrote:
> At 11:53 AM 2/5/2003, Jeff Trawick wrote:
>
> >on APR not providing a string which tells what type of processing failed:
> >
> >With no string from APR, you don't know if, for example, the failure
> was EPERM because
> >
> >a) permissions on working directory were bad
> >b) permissions on executable were bad
> >c) no permission to raise rlimits to specified value
> >
> >A certain large class of users would really benefit from such
> information, even if it is not in their native language and they have
> to feed it into google. (But surely if we'd eventually translate
> other APR strings then an infrastructure would be in place.)
>
>
> Oh, I agree there. I just wouldn't stuff the program name and do
> the string munging, let the user format it all. What about simply
> passing a status flag so the callback can actually react to those
> different cases (e.g. APR_PROC_FAIL_CWD, APR_PROC_FAIL_LOAD,
> APR_PROC_FAIL_PERMS etc)?
I did consider different enums to cover different processing steps that
might fail but I'd then want APR to give me a stringify function so I
didn't have to check the enums myself and potentially have to make my
program smarter when APR added a new potential failure point. Too
complicating.
>
> Of course, also pass the actual apr_status_t from the operation
> that failed.
yep, have to have that
Re: [PATCH] letting the app do something useful when
apr_proc_create() fails in the child process
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 11:53 AM 2/5/2003, Jeff Trawick wrote:
>William A. Rowe, Jr. wrote:
>
>>At 08:59 AM 2/5/2003, Jeff Trawick wrote:
>>
>>I think that you, Justin, and I agree that there is no need for a feature test macro. That's cool.
Yup - all on the same page here.
>>But what about simply an apr_proc_create_ex() function that accepts
>>the callback. I suspect this might be cleaner, because it will be easier
>>to find code that failed to provide such a callback (which is really a bug,
>>IMHO.) Like socket_create we can drop the _ex from APR 1.0 and always
>>add that arguement to socket_create. The code will probably be easier
>>to read, in any case. This should be supported on all platforms, it's simply
>>a noop where fork() wasn't involved. Either way, though...
>
>I'd like to see some +1s for this from others. I'm not sure that it is a bug not to have the function. I'm not really opposed, I simply would choose to continue adding process attributes rather than more parameters.
That's fine. Docs should spell out that the callback_set() fn replaces the
prior callback, or that NULL can be passed to callback_set() to unset it.
>>Please make the callback take the apr_status_t result, the apr_procattr_t
>>that failed and a pool. Let the caller format it into a message if they like,
>>rather than creating more language-specific strings within apr itself. The
>>callback should be able to take anything back out of the apr_procattr_t that
>>it's interested in. And they should be able to take their context back out
>>of the apr_procattr_t rather than another userdata field. (Heck, they could
>>even create pool data if they needed.)
>
>on passing apr_procattr_t:
>
>apr_procattr_t is private, so we'd have to add accessor functions for that to work. Otherwise, it isn't useful.
Well, since I simply want the stderr pipe back in my hands so I can emit
it to the right place, accessors seem to make some sense. We can probably
make a pretty short list of these that are useful to such callbacks.
>on not having a parameter for specifying userdata:
>
>The app could attach userdata to a pool and we'd document that the pool better be the one passed to apr_proc_create() since that is the one we'd be passing to the child error function. (or the one passed to apr_procattr_create())
That was my thought, and docs should be clear on the point, yes.
>Somehow it seems simple to document that what you pass to apr_procattr_child_errfn() set is what you get in the error function. Though if we go with the added error function parameter to apr_proc_create_ex() then I see how it is important to avoid adding even another parameter.
Oh, certainly contexts aren't hard to document. I was just looking for
the cheap-and-easy shortcut of using the procattr we already have.
>on APR not providing a string which tells what type of processing failed:
>
>With no string from APR, you don't know if, for example, the failure was EPERM because
>
>a) permissions on working directory were bad
>b) permissions on executable were bad
>c) no permission to raise rlimits to specified value
>
>A certain large class of users would really benefit from such information, even if it is not in their native language and they have to feed it into google. (But surely if we'd eventually translate other APR strings then an infrastructure would be in place.)
Oh, I agree there. I just wouldn't stuff the program name and do
the string munging, let the user format it all. What about simply
passing a status flag so the callback can actually react to those
different cases (e.g. APR_PROC_FAIL_CWD, APR_PROC_FAIL_LOAD,
APR_PROC_FAIL_PERMS etc)?
Of course, also pass the actual apr_status_t from the operation
that failed.
Bill
Re: [PATCH] letting the app do something useful when
apr_proc_create() fails in the child process
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Wednesday, February 5, 2003 12:53 PM -0500 Jeff Trawick
<tr...@attglobal.net> wrote:
> I'd like to see some +1s for this from others. I'm not sure that
> it is a bug not to have the function. I'm not really opposed, I
> simply would choose to continue adding process attributes rather
> than more parameters.
I agree with Jeff. I don't think we need to continue to add
parameters to apr_proc_create. IIRC, we can reuse procattr across
multiple apr_proc_create invocations (might be wrong). Therefore,
the presence (or absence) of a child error function makes sense to be
where it is now. Functions with really, really, really, really long
argument lists are a bit unwieldly.
It isn't a big deal if you don't have a child error function, and I
bet the common case would be not to have it anyway. No need to
continue to pollute apr_proc_create. -- justin
Re: [PATCH] letting the app do something useful when apr_proc_create()
fails in the child process
Posted by Jeff Trawick <tr...@attglobal.net>.
William A. Rowe, Jr. wrote:
> At 08:59 AM 2/5/2003, Jeff Trawick wrote:
>
> >Any concerns, particularly with respect to how the app determines if
> the feature is available? I think it would be fine to make the
> feature always available but to simply note that on some platforms the
> application-specified error function would never be called.
>
>
> Right... because those platforms that don't 'react' to the child_errfn are
> *generally* the platforms that will return a failure result from invoking
> apr_proc_create. So we don't really lose anything by introducing your
> new schema, and we have everything to gain. ++1
I think that you, Justin, and I agree that there is no need for a
feature test macro. That's cool.
> But what about simply an apr_proc_create_ex() function that accepts
> the callback. I suspect this might be cleaner, because it will be easier
> to find code that failed to provide such a callback (which is really a
> bug,
> IMHO.) Like socket_create we can drop the _ex from APR 1.0 and always
> add that arguement to socket_create. The code will probably be easier
> to read, in any case. This should be supported on all platforms, it's
> simply
> a noop where fork() wasn't involved. Either way, though...
I'd like to see some +1s for this from others. I'm not sure that it is
a bug not to have the function. I'm not really opposed, I simply would
choose to continue adding process attributes rather than more parameters.
> Please make the callback take the apr_status_t result, the apr_procattr_t
> that failed and a pool. Let the caller format it into a message if
> they like,
> rather than creating more language-specific strings within apr itself.
> The
> callback should be able to take anything back out of the
> apr_procattr_t that
> it's interested in. And they should be able to take their context
> back out
> of the apr_procattr_t rather than another userdata field. (Heck, they
> could
> even create pool data if they needed.)
on passing apr_procattr_t:
apr_procattr_t is private, so we'd have to add accessor functions for
that to work. Otherwise, it isn't useful.
--/--
on not having a parameter for specifying userdata:
How do you get context back from apr_procattr_t? By adding
apr_procattr_userdata_[get|set]()?
The app could attach userdata to a pool and we'd document that the pool
better be the one passed to apr_proc_create() since that is the one we'd
be passing to the child error function. (or the one passed to
apr_procattr_create())
Somehow it seems simple to document that what you pass to
apr_procattr_child_errfn() set is what you get in the error function.
Though if we go with the added error function parameter to
apr_proc_create_ex() then I see how it is important to avoid adding even
another parameter.
--/--
on APR not providing a string which tells what type of processing failed:
With no string from APR, you don't know if, for example, the failure was
EPERM because
a) permissions on working directory were bad
b) permissions on executable were bad
c) no permission to raise rlimits to specified value
A certain large class of users would really benefit from such
information, even if it is not in their native language and they have to
feed it into google. (But surely if we'd eventually translate other APR
strings then an infrastructure would be in place.)
--/--
Thanks,
Jeff
Re: [PATCH] letting the app do something useful when
apr_proc_create() fails in the child process
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 08:59 AM 2/5/2003, Jeff Trawick wrote:
>Any concerns, particularly with respect to how the app determines if the feature is available? I think it would be fine to make the feature always available but to simply note that on some platforms the application-specified error function would never be called.
Right... because those platforms that don't 'react' to the child_errfn are
*generally* the platforms that will return a failure result from invoking
apr_proc_create. So we don't really lose anything by introducing your
new schema, and we have everything to gain. ++1
But what about simply an apr_proc_create_ex() function that accepts
the callback. I suspect this might be cleaner, because it will be easier
to find code that failed to provide such a callback (which is really a bug,
IMHO.) Like socket_create we can drop the _ex from APR 1.0 and always
add that arguement to socket_create. The code will probably be easier
to read, in any case. This should be supported on all platforms, it's simply
a noop where fork() wasn't involved. Either way, though...
Please make the callback take the apr_status_t result, the apr_procattr_t
that failed and a pool. Let the caller format it into a message if they like,
rather than creating more language-specific strings within apr itself. The
callback should be able to take anything back out of the apr_procattr_t that
it's interested in. And they should be able to take their context back out
of the apr_procattr_t rather than another userdata field. (Heck, they could
even create pool data if they needed.)
Again, great idea!
Bill