You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by William A Rowe Jr <wr...@rowe-clan.net> on 2015/11/20 00:24:25 UTC
Re: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c
It wouldn't hurt to change this though, the tokens are generally
represented in lowercase, and this could avoid case folding I suppose.
How often do we see value tokens as upper case from httpd? Let's be
consistent although it isn't strictly required.
On Thu, Nov 19, 2015 at 3:54 PM, <ja...@apache.org> wrote:
> Author: jailletc36
> Date: Thu Nov 19 21:54:48 2015
> New Revision: 1715294
>
> URL: http://svn.apache.org/viewvc?rev=1715294&view=rev
> Log:
> Revert r1715289 (Connection header field should use "upgrade" instead of
> "Upgrade")
>
> This is case-insensitive, so no need for such a change.
>
> Modified:
> httpd/httpd/trunk/server/core.c
>
> Modified: httpd/httpd/trunk/server/core.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1715294&r1=1715293&r2=1715294&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/server/core.c (original)
> +++ httpd/httpd/trunk/server/core.c Thu Nov 19 21:54:48 2015
> @@ -5379,7 +5379,7 @@ static int core_upgrade_handler(request_
> /* Let the client know what we are upgrading to. */
> apr_table_clear(r->headers_out);
> apr_table_setn(r->headers_out, "Upgrade", protocol);
> - apr_table_setn(r->headers_out, "Connection",
> "upgrade");
> + apr_table_setn(r->headers_out, "Connection",
> "Upgrade");
>
> r->status = HTTP_SWITCHING_PROTOCOLS;
> r->status_line = ap_get_status_line(r->status);
> @@ -5404,7 +5404,7 @@ static int core_upgrade_handler(request_
> if (upgrades && upgrades->nelts > 0) {
> char *protocols = apr_array_pstrcat(r->pool, upgrades, ',');
> apr_table_setn(r->headers_out, "Upgrade", protocols);
> - apr_table_setn(r->headers_out, "Connection", "upgrade");
> + apr_table_setn(r->headers_out, "Connection", "Upgrade");
> }
> }
>
>
>
>
Re: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c
Posted by Marion & Christophe JAILLET <ch...@wanadoo.fr>.
That was I first thought too.
When I looked at r1715255, I noticed:
const char *conn = apr_table_get(r->headers_in, "Connection");
if (ap_find_token(r->pool, conn, "upgrade")) {
and looked for inconsistencies.
When digging deeper, I found that "Connection Upgrade" was used in most
cases in httpd. That's why I reverted.
The only places where I found the lowercase version are:
core.c:5366: if (ap_find_token(r->pool, conn, "upgrade")) {
ssl_engine_kernel.c:1344: apr_table_mergen(r->headers_out,
"Connection", "upgrade");
http2: in different places
So, for consistency, it's maybe these places that should be updated???
In any case, I don't think that it would avoid some case folding.
For "Connection upgrade", the only places I've found that process it is
the one given above (core.c:5366)
In this case, taking advantage of a lower case version would require to
tweak ap_find_token.
This would, IMHO, add complexity to the code and would be worse for the
common case (i.e. if what we have does not match what we test, we would
test twice)
If having upgrade in lower case makes it way, other places should also
be looked at:
Upgrade: WebSocket vs websocket
CJ
Le 20/11/2015 00:24, William A Rowe Jr a écrit :
> It wouldn't hurt to change this though, the tokens are generally
> represented in lowercase, and this could avoid case folding I suppose.
>
> How often do we see value tokens as upper case from httpd? Let's be
> consistent although it isn't strictly required.
>
>
>
> On Thu, Nov 19, 2015 at 3:54 PM, <jailletc36@apache.org
> <ma...@apache.org>> wrote:
>
> Author: jailletc36
> Date: Thu Nov 19 21:54:48 2015
> New Revision: 1715294
>
> URL: http://svn.apache.org/viewvc?rev=1715294&view=rev
> Log:
> Revert r1715289 (Connection header field should use "upgrade"
> instead of "Upgrade")
>
> This is case-insensitive, so no need for such a change.
>
> Modified:
> httpd/httpd/trunk/server/core.c
>
> Modified: httpd/httpd/trunk/server/core.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1715294&r1=1715293&r2=1715294&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/core.c (original)
> +++ httpd/httpd/trunk/server/core.c Thu Nov 19 21:54:48 2015
> @@ -5379,7 +5379,7 @@ static int core_upgrade_handler(request_
> /* Let the client know what we are upgrading
> to. */
> apr_table_clear(r->headers_out);
> apr_table_setn(r->headers_out, "Upgrade",
> protocol);
> - apr_table_setn(r->headers_out, "Connection",
> "upgrade");
> + apr_table_setn(r->headers_out, "Connection",
> "Upgrade");
>
> r->status = HTTP_SWITCHING_PROTOCOLS;
> r->status_line = ap_get_status_line(r->status);
> @@ -5404,7 +5404,7 @@ static int core_upgrade_handler(request_
> if (upgrades && upgrades->nelts > 0) {
> char *protocols = apr_array_pstrcat(r->pool,
> upgrades, ',');
> apr_table_setn(r->headers_out, "Upgrade", protocols);
> - apr_table_setn(r->headers_out, "Connection", "upgrade");
> + apr_table_setn(r->headers_out, "Connection", "Upgrade");
> }
> }
>
>
>
>
Re: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Nov 20, 2015 at 3:13 AM, Bert Huijben <be...@qqmail.nl> wrote:
>
> > -----Original Message-----
> > From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
> > Sent: vrijdag 20 november 2015 10:11
> > To: dev@httpd.apache.org
> > Subject: Re: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c
> >
> > +1 for lowercasing and pls backport, since it just arrived as is in
> 2.4.x,
> i think.
>
> > > - apr_table_setn(r->headers_out, "Connection",
> "upgrade");
> > > + apr_table_setn(r->headers_out, "Connection",
> "Upgrade");
>
> As the 'Connection' header technically refers to the 'Upgrade' header I
> would paint both sheds in the same color.
It does, yes, That's part of my motivation, the Key (field title) is
generally
capitalized, the values are generally lowercase, and in this case upgrade
does appear twice, so it helps slightly to distinguish the two.
Referring back it has appeared both ways in the specs, but RFC 7230
has buried RFC 2817 :)
http://tools.ietf.org/html/rfc7230#page-57
My inclination is obvious but I don't feel strongly enough to patch this
myself, will let the others do so. For reference, we actually followed
the RFC 2817 conventions in 2.2...
ssl_engine_io.c:#define CONNECTION_HEADER "Connection: Upgrade"
ssl_engine_kernel.c: apr_table_setn(r->err_headers_out,
"Connection", "Upgrade");
and it now appears both ways in the code. /shrug
ssl_engine_kernel.c:#define CONNECTION_HEADER "Connection: Upgrade"
ssl_engine_kernel.c: apr_table_setn(r->err_headers_out,
"Connection", "Upgrade");
ssl_engine_kernel.c: * connection, and this isn't a subrequest, send an
Upgrade
ssl_engine_kernel.c: apr_table_mergen(r->headers_out, "Connection",
"upgrade");
RE: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Eissing [mailto:stefan.eissing@greenbytes.de]
> Sent: vrijdag 20 november 2015 10:11
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c
>
> +1 for lowercasing and pls backport, since it just arrived as is in 2.4.x,
i think.
<snip>
> > apr_table_clear(r->headers_out);
> > apr_table_setn(r->headers_out, "Upgrade",
protocol);
> > - apr_table_setn(r->headers_out, "Connection",
"upgrade");
> > + apr_table_setn(r->headers_out, "Connection",
"Upgrade");
As the 'Connection' header technically refers to the 'Upgrade' header I
would paint both sheds in the same color.
Either would be fine by me.
Bert
Re: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c
Posted by Stefan Eissing <st...@greenbytes.de>.
+1 for lowercasing and pls backport, since it just arrived as is in 2.4.x, i think.
> Am 20.11.2015 um 00:24 schrieb William A Rowe Jr <wr...@rowe-clan.net>:
>
> It wouldn't hurt to change this though, the tokens are generally represented in lowercase, and this could avoid case folding I suppose.
>
> How often do we see value tokens as upper case from httpd? Let's be consistent although it isn't strictly required.
>
>
>
> On Thu, Nov 19, 2015 at 3:54 PM, <ja...@apache.org> wrote:
> Author: jailletc36
> Date: Thu Nov 19 21:54:48 2015
> New Revision: 1715294
>
> URL: http://svn.apache.org/viewvc?rev=1715294&view=rev
> Log:
> Revert r1715289 (Connection header field should use "upgrade" instead of "Upgrade")
>
> This is case-insensitive, so no need for such a change.
>
> Modified:
> httpd/httpd/trunk/server/core.c
>
> Modified: httpd/httpd/trunk/server/core.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1715294&r1=1715293&r2=1715294&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/core.c (original)
> +++ httpd/httpd/trunk/server/core.c Thu Nov 19 21:54:48 2015
> @@ -5379,7 +5379,7 @@ static int core_upgrade_handler(request_
> /* Let the client know what we are upgrading to. */
> apr_table_clear(r->headers_out);
> apr_table_setn(r->headers_out, "Upgrade", protocol);
> - apr_table_setn(r->headers_out, "Connection", "upgrade");
> + apr_table_setn(r->headers_out, "Connection", "Upgrade");
>
> r->status = HTTP_SWITCHING_PROTOCOLS;
> r->status_line = ap_get_status_line(r->status);
> @@ -5404,7 +5404,7 @@ static int core_upgrade_handler(request_
> if (upgrades && upgrades->nelts > 0) {
> char *protocols = apr_array_pstrcat(r->pool, upgrades, ',');
> apr_table_setn(r->headers_out, "Upgrade", protocols);
> - apr_table_setn(r->headers_out, "Connection", "upgrade");
> + apr_table_setn(r->headers_out, "Connection", "Upgrade");
> }
> }
>
>
>
>