Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Errors-To: <wrowe@rowe-clan.net>
Message-Id: <5.2.0.9.2.20030205121951.013983c8@pop3.rowe-clan.net>
Date: Wed, 05 Feb 2003 12:27:05 -0600
To: trawick@attglobal.net
From: "William A. Rowe, Jr." <wrowe@rowe-clan.net>
Subject: Re: [PATCH] letting the app do something useful when
  apr_proc_create() fails in the child process
Cc: dev@apr.apache.org
In-Reply-To: <3E414F90.4030002@attglobal.net>
References: <5.2.0.9.2.20030205093325.02b7f828@pop3.rowe-clan.net>
 <5.2.0.9.2.20030205093325.02b7f828@pop3.rowe-clan.net>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"

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