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
