apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bill Stoddard <b...@wstoddard.com>
Subject Re: [PATCH] letting the app do something useful when apr_proc_create() fails in the child process
Date Wed, 05 Feb 2003 15:33:27 GMT
+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. */
>      }



Mime
View raw message