Re: [PATCH] letting the app do something useful when apr_proc_create() fails in the child process

["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 

Re: [PATCH] letting the app do something useful when apr_proc_create() fails in the child process

["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

[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

["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

[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

[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