OSDir


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: svn commit: r1840265 - in /httpd/httpd/trunk: include/ap_mmn.h include/util_filter.h modules/http/http_request.c server/core_filters.c server/util_filter.c


I'd suggest you did a better job in the commit log explaning this than in the doxygen where it really is needed. 

Private declares don't belong in util_filter.h IMO.

On Thu, Sep 6, 2018, 17:48 <ylavic@xxxxxxxxxx> wrote:
Author: ylavic
Date: Thu Sep  6 22:48:28 2018
New Revision: 1840265

URL: http://svn.apache.org/viewvc?rev=1840265&view=rev
Log:
Follow up to r1840149: core input filter pending data.

Since r1840149 ap_core_input_filter() can't use use f->[priv->]bb directly, so
ap_filter_input_pending() stopped accounting for its pending data.

But ap_core_input_filter() can't (and doesn't need to) setaside its socket
bucket, so ap_filter_setaside_brigade() is not an option. This commit adds
ap_filter_adopt_brigade() which simply moves the given buckets (brigade) into
f->priv->bb, and since this is not something to be done blindly (the buckets
need to have c->pool/bucket_alloc lifetime, which is the case in the core
filter) the function is not AP_DECLAREd/exported thus can be used in core only.

With ap_filter_adopt_brigade() and ap_filter_reinstate_brigade(), the core
input is now ap_filter_input_pending() friendly.

Also, ap_filter_recycle() is no more part of the API (AP_DECLARE removed too),
there really is no point to call it outside core code. MAJOR bumped once again
because of this.

Modified:
    httpd/httpd/trunk/include/ap_mmn.h
    httpd/httpd/trunk/include/util_filter.h
    httpd/httpd/trunk/modules/http/http_request.c
    httpd/httpd/trunk/server/core_filters.c
    httpd/httpd/trunk/server/util_filter.c

Modified: httpd/httpd/trunk/include/ap_mmn.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ap_mmn.h?rev=1840265&r1=1840264&r2=1840265&view=diff
==============================================================================
--- httpd/httpd/trunk/include/ap_mmn.h (original)
+++ httpd/httpd/trunk/include/ap_mmn.h Thu Sep  6 22:48:28 2018
@@ -606,12 +606,13 @@
  *                         ap_acquire_brigade()/ap_release_brigade(), and
  *                         in ap_filter_t replace pending/bb/deferred_pool
  *                         fields by struct ap_filter_private *priv
+ * 20180906.1 (2.5.1-dev)  Don't export ap_filter_recycle() anymore
  */

 #define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */

 #ifndef MODULE_MAGIC_NUMBER_MAJOR
-#define MODULE_MAGIC_NUMBER_MAJOR 20180905
+#define MODULE_MAGIC_NUMBER_MAJOR 20180906
 #endif
 #define MODULE_MAGIC_NUMBER_MINOR 1                 /* 0...n */


Modified: httpd/httpd/trunk/include/util_filter.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/util_filter.h?rev=1840265&r1=1840264&r2=1840265&view=diff
==============================================================================
--- httpd/httpd/trunk/include/util_filter.h (original)
+++ httpd/httpd/trunk/include/util_filter.h Thu Sep  6 22:48:28 2018
@@ -596,6 +596,16 @@ AP_DECLARE(int) ap_filter_prepare_brigad
 AP_DECLARE(apr_status_t) ap_filter_setaside_brigade(ap_filter_t *f,
                                                     apr_bucket_brigade *bb);

+/*
+ * Adopt a bucket brigade as is (no setaside nor copy).
+ * @param f The current filter
+ * @param bb The bucket brigade adopted.  This brigade is always empty
+ *          on return
+ * @remark httpd internal, not exported, needed by
+ *               ap_core_input_filter
+ */
+void ap_filter_adopt_brigade(ap_filter_t *f, apr_bucket_brigade *bb);
+
 /**
  * Reinstate a brigade setaside earlier, and calculate the amount of data we
  * should write based on the presence of flush buckets, size limits on in
@@ -656,14 +666,17 @@ AP_DECLARE_NONSTD(int) ap_filter_output_
  */
 AP_DECLARE_NONSTD(int) ap_filter_input_pending(conn_rec *c);

-/**
+/*
  * Recycle removed request filters so that they can be reused for filters
  * added later on the same connection. This typically should happen after
  * each request handling.
  *
  * @param c The connection.
+ * @remark httpd internal, not exported, needed by
+ *         ap_process_request_after_handler
+ *         
  */
-AP_DECLARE(void) ap_filter_recycle(conn_rec *c);
+void ap_filter_recycle(conn_rec *c);

 /**
  * Flush function for apr_brigade_* calls.  This calls ap_pass_brigade

Modified: httpd/httpd/trunk/modules/http/http_request.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_request.c?rev=1840265&r1=1840264&r2=1840265&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http/http_request.c (original)
+++ httpd/httpd/trunk/modules/http/http_request.c Thu Sep  6 22:48:28 2018
@@ -402,10 +402,7 @@ AP_DECLARE(void) ap_process_request_afte
     (void)ap_check_pipeline(c, bb, DEFAULT_LIMIT_BLANK_LINES);

     ap_release_brigade(c, bb);
-
-    if (!c->aborted) {
-        ap_filter_recycle(c);
-    }
+    ap_filter_recycle(c);

     if (c->cs) {
         if (c->aborted) {

Modified: httpd/httpd/trunk/server/core_filters.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core_filters.c?rev=1840265&r1=1840264&r2=1840265&view=diff
==============================================================================
--- httpd/httpd/trunk/server/core_filters.c (original)
+++ httpd/httpd/trunk/server/core_filters.c Thu Sep  6 22:48:28 2018
@@ -94,7 +94,7 @@ apr_status_t ap_core_input_filter(ap_fil
                                   ap_input_mode_t mode, apr_read_type_e block,
                                   apr_off_t readbytes)
 {
-    apr_status_t rv;
+    apr_status_t rv = APR_SUCCESS;
     core_net_rec *net = f->ctx;
     core_ctx_t *ctx = net->in_ctx;
     const char *str;
@@ -124,8 +124,11 @@ apr_status_t ap_core_input_filter(ap_fil
         if (rv != APR_SUCCESS)
             return rv;
     }
-    else if (APR_BRIGADE_EMPTY(ctx->bb)) {
-        return APR_EOF;
+    else {
+        ap_filter_reinstate_brigade(f, ctx->bb, NULL);
+        if (APR_BRIGADE_EMPTY(ctx->bb)) {
+            return APR_EOF;
+        }
     }

     /* ### This is bad. */
@@ -151,7 +154,7 @@ apr_status_t ap_core_input_filter(ap_fil
         if (APR_STATUS_IS_EAGAIN(rv) && block == APR_NONBLOCK_READ) {
             rv = APR_SUCCESS;
         }
-        return rv;
+        goto cleanup;
     }

     /* ### AP_MODE_PEEK is a horrific name for this mode because we also
@@ -171,15 +174,16 @@ apr_status_t ap_core_input_filter(ap_fil
          * mean that there is another request, just a blank line.
          */
         while (1) {
-            if (APR_BRIGADE_EMPTY(ctx->bb))
-                return APR_EOF;
+            if (APR_BRIGADE_EMPTY(ctx->bb)) {
+                rv = APR_EOF;
+                goto cleanup;
+            }

             e = APR_BRIGADE_FIRST(ctx->bb);
-
             rv = apr_bucket_read(e, &str, &len, APR_NONBLOCK_READ);
-
-            if (rv != APR_SUCCESS)
-                return rv;
+            if (rv != APR_SUCCESS) {
+                goto cleanup;
+            }

             c = str;
             while (c < str + len) {
@@ -188,7 +192,7 @@ apr_status_t ap_core_input_filter(ap_fil
                 else if (*c == APR_ASCII_CR && *(c + 1) == APR_ASCII_LF)
                     c += 2;
                 else
-                    return APR_SUCCESS;
+                    goto cleanup;
             }

             /* If we reach here, we were a bucket just full of CRLFs, so
@@ -196,7 +200,9 @@ apr_status_t ap_core_input_filter(ap_fil
             /* FIXME: Is this the right thing to do in the core? */
             apr_bucket_delete(e);
         }
-        return APR_SUCCESS;
+
+        /* UNREACHABLE */
+        ap_assert(0);
     }

     /* If mode is EXHAUSTIVE, we want to just read everything until the end
@@ -222,7 +228,9 @@ apr_status_t ap_core_input_filter(ap_fil
          * must be EOS. */
         e = apr_bucket_eos_create(f->c->bucket_alloc);
         APR_BRIGADE_INSERT_TAIL(b, e);
-        return APR_SUCCESS;
+
+        rv = APR_SUCCESS;
+        goto cleanup;
     }

     /* read up to the amount they specified. */
@@ -233,14 +241,13 @@ apr_status_t ap_core_input_filter(ap_fil

         e = APR_BRIGADE_FIRST(ctx->bb);
         rv = apr_bucket_read(e, &str, &len, block);
-
-        if (APR_STATUS_IS_EAGAIN(rv) && block == APR_NONBLOCK_READ) {
-            /* getting EAGAIN for a blocking read is an error; for a
-             * non-blocking read, return an empty brigade. */
-            return APR_SUCCESS;
-        }
-        else if (rv != APR_SUCCESS) {
-            return rv;
+        if (rv != APR_SUCCESS) {
+            if (APR_STATUS_IS_EAGAIN(rv) && block == APR_NONBLOCK_READ) {
+                /* getting EAGAIN for a blocking read is an error; not for a
+                 * non-blocking read, return an empty brigade. */
+                rv = APR_SUCCESS;
+            }
+            goto cleanup;
         }
         else if (block == APR_BLOCK_READ && len == 0) {
             /* We wanted to read some bytes in blocking mode.  We read
@@ -257,14 +264,13 @@ apr_status_t ap_core_input_filter(ap_fil
                 e = apr_bucket_eos_create(f->c->bucket_alloc);
                 APR_BRIGADE_INSERT_TAIL(b, e);
             }
-            return APR_SUCCESS;
+            goto cleanup;
         }

         /* Have we read as much data as we wanted (be greedy)? */
         if (len < readbytes) {
             apr_size_t bucket_len;

-            rv = APR_SUCCESS;
             /* We already registered the data in e in len */
             e = APR_BUCKET_NEXT(e);
             while ((len < readbytes) && (rv == APR_SUCCESS)
@@ -298,7 +304,7 @@ apr_status_t ap_core_input_filter(ap_fil

         rv = apr_brigade_partition(ctx->bb, readbytes, &e);
         if (rv != APR_SUCCESS) {
-            return rv;
+            goto cleanup;
         }

         /* Must do move before CONCAT */
@@ -316,7 +322,7 @@ apr_status_t ap_core_input_filter(ap_fil
             {
                 rv = apr_bucket_copy(e, &copy_bucket);
                 if (rv != APR_SUCCESS) {
-                    return rv;
+                    goto cleanup;
                 }
                 APR_BRIGADE_INSERT_TAIL(b, copy_bucket);
             }
@@ -325,7 +331,10 @@ apr_status_t ap_core_input_filter(ap_fil
         /* Take what was originally there and place it back on ctx->bb */
         APR_BRIGADE_CONCAT(ctx->bb, ctx->tmpbb);
     }
-    return APR_SUCCESS;
+
+cleanup:
+    ap_filter_adopt_brigade(f, ctx->bb);
+    return rv;
 }

 static apr_status_t send_brigade_nonblocking(apr_socket_t *s,

Modified: httpd/httpd/trunk/server/util_filter.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_filter.c?rev=1840265&r1=1840264&r2=1840265&view=diff
==============================================================================
--- httpd/httpd/trunk/server/util_filter.c (original)
+++ httpd/httpd/trunk/server/util_filter.c Thu Sep  6 22:48:28 2018
@@ -321,7 +321,8 @@ static struct ap_filter_conn_ctx *get_co
     return x;
 }

-static void make_spare_ring(struct spare_ring **ring, apr_pool_t *p)
+static APR_INLINE
+void make_spare_ring(struct spare_ring **ring, apr_pool_t *p)
 {
     if (!*ring) {
         *ring = apr_palloc(p, sizeof(**ring));
@@ -403,7 +404,7 @@ static apr_status_t request_filter_clean
     return APR_SUCCESS;
 }

-AP_DECLARE(void) ap_filter_recycle(conn_rec *c)
+void ap_filter_recycle(conn_rec *c)
 {
     struct ap_filter_conn_ctx *x = c->filter_conn_ctx;

@@ -983,6 +984,22 @@ AP_DECLARE(apr_status_t) ap_filter_setas
     return APR_SUCCESS;
 }

+void ap_filter_adopt_brigade(ap_filter_t *f, apr_bucket_brigade *bb)
+{
+    struct ap_filter_private *fp = f->priv;
+
+    ap_log_cerror(APLOG_MARK, APLOG_TRACE6, 0, f->c,
+                  "adopt %s brigade to %s brigade in '%s' output filter",
+                  APR_BRIGADE_EMPTY(bb) ? "empty" : "full",
+                  (!fp->bb || APR_BRIGADE_EMPTY(fp->bb)) ? "empty" : "full",
+                  f->frec->name);
+
+    if (!APR_BRIGADE_EMPTY(bb)) {
+        ap_filter_prepare_brigade(f);
+        APR_BRIGADE_CONCAT(fp->bb, bb);
+    }
+}
+
 AP_DECLARE(apr_status_t) ap_filter_reinstate_brigade(ap_filter_t *f,
                                                      apr_bucket_brigade *bb,
                                                      apr_bucket **flush_upto)
@@ -1308,6 +1325,7 @@ AP_DECLARE_NONSTD(apr_status_t) ap_fprin
     va_end(args);
     return rv;
 }
+
 AP_DECLARE(void) ap_filter_protocol(ap_filter_t *f, unsigned int flags)
 {
     f->frec->proto_flags = flags ;