You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Eric Covener <co...@gmail.com> on 2018/04/11 19:14:26 UTC
Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c
> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Wed Apr 11 19:11:52 2018
> @@ -459,6 +459,8 @@ typedef struct {
> char secret[PROXY_WORKER_MAX_SECRET_SIZE]; /* authentication secret (e.g. AJP13) */
> char upgrade[PROXY_WORKER_MAX_SCHEME_SIZE];/* upgrade protocol used by mod_proxy_wstunnel */
> char hostname_ex[PROXY_RFC1035_HOSTNAME_SIZE]; /* RFC1035 compliant version of the remote backend address */
> + apr_size_t response_field_size; /* Size of proxy response buffer in bytes. */
> + unsigned int response_field_size_set:1;
> } proxy_worker_shared;
If this is for trunk only, should I move the bit field up and call it
major? I don't plan to backport it.
Whether I move it or not, should I reserve the next range of bytes after it?
Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Jun 26, 2018 at 8:21 AM, Eric Covener <co...@gmail.com> wrote:
> On Tue, Jun 26, 2018 at 9:02 AM Yann Ylavic <yl...@gmail.com> wrote:
> >
> > On Tue, Jun 26, 2018 at 2:48 PM, Eric Covener <co...@gmail.com> wrote:
> > > On Tue, Jun 26, 2018 at 8:37 AM Yann Ylavic <yl...@gmail.com>
> wrote:
> > >>
> > >> On Mon, Jun 25, 2018 at 10:49 PM, Eric Covener <co...@gmail.com>
> wrote:
> > >> >
> > >> > Bill or Yann, do you remember the specific gotcha with setting aside
> > >> > addl bits and re-using them later?
> > >>
> > >> I've never thought it was an issue (re compat) to add some bit(s) in a
> > >> bitfield if there is a hole, wherever this field is in the struct.
> > >> It doesn't change the size and there is no way for the user to have
> > >> used the address of any bit in the first place (it can't break
> > >> anything IMHO).
> > >> Bill objected to this, I can't remember why (and he is in better
> > >> position to explain it), status quo so far...
> > >>
> > >
> > > Just to be clear, I am talking about claiming the lost ones _before_
> > > the struct is further extended with a reserved :31 (or whatever) as
> > > opposed to going back and claiming gaps from the past. Maybe the
> > > former is OK.
> >
> > I think "reserved" or not doesn't change anything, name it or not the
> > hole is there in any case.
> > Why extending firstbit:1;reserved:31 to e.g.
> > firstbit:1;secondbit:1;reserved:30 would be more compatible than the
> > implicit firstbit:1;secondbit:1 ?
> > Both should be allowed IMHO.
>
> Speculating only, it seems like there is a gradient of risk:
>
> - Old unused bits we try to repurpose that someone is squatting on
> - New unused bits we try to claim now (this is the case for the
> recent single bit field)
> - Unused bits that are explicitly marked as reserved when the first
> bit field is defined.
>
My sole concern was whether the addition of c below...
struct {
int a:2;
int b:2;
int c:2; /* Added */
}
...ever assured that the two bits of c will not displace a or b, and this
has never been a concern on Intel implementatins. Actually Eric you
have more background on big endian and odd word size architectures
than I do :) I've never found a reference that assures us of this, maybe
things have changed in very recent C specs?
Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c
Posted by Eric Covener <co...@gmail.com>.
On Tue, Jun 26, 2018 at 9:02 AM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Tue, Jun 26, 2018 at 2:48 PM, Eric Covener <co...@gmail.com> wrote:
> > On Tue, Jun 26, 2018 at 8:37 AM Yann Ylavic <yl...@gmail.com> wrote:
> >>
> >> On Mon, Jun 25, 2018 at 10:49 PM, Eric Covener <co...@gmail.com> wrote:
> >> >
> >> > Bill or Yann, do you remember the specific gotcha with setting aside
> >> > addl bits and re-using them later?
> >>
> >> I've never thought it was an issue (re compat) to add some bit(s) in a
> >> bitfield if there is a hole, wherever this field is in the struct.
> >> It doesn't change the size and there is no way for the user to have
> >> used the address of any bit in the first place (it can't break
> >> anything IMHO).
> >> Bill objected to this, I can't remember why (and he is in better
> >> position to explain it), status quo so far...
> >>
> >
> > Just to be clear, I am talking about claiming the lost ones _before_
> > the struct is further extended with a reserved :31 (or whatever) as
> > opposed to going back and claiming gaps from the past. Maybe the
> > former is OK.
>
> I think "reserved" or not doesn't change anything, name it or not the
> hole is there in any case.
> Why extending firstbit:1;reserved:31 to e.g.
> firstbit:1;secondbit:1;reserved:30 would be more compatible than the
> implicit firstbit:1;secondbit:1 ?
> Both should be allowed IMHO.
Speculating only, it seems like there is a gradient of risk:
- Old unused bits we try to repurpose that someone is squatting on
- New unused bits we try to claim now (this is the case for the
recent single bit field)
- Unused bits that are explicitly marked as reserved when the first
bit field is defined.
Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Jun 26, 2018 at 2:48 PM, Eric Covener <co...@gmail.com> wrote:
> On Tue, Jun 26, 2018 at 8:37 AM Yann Ylavic <yl...@gmail.com> wrote:
>>
>> On Mon, Jun 25, 2018 at 10:49 PM, Eric Covener <co...@gmail.com> wrote:
>> >
>> > Bill or Yann, do you remember the specific gotcha with setting aside
>> > addl bits and re-using them later?
>>
>> I've never thought it was an issue (re compat) to add some bit(s) in a
>> bitfield if there is a hole, wherever this field is in the struct.
>> It doesn't change the size and there is no way for the user to have
>> used the address of any bit in the first place (it can't break
>> anything IMHO).
>> Bill objected to this, I can't remember why (and he is in better
>> position to explain it), status quo so far...
>>
>
> Just to be clear, I am talking about claiming the lost ones _before_
> the struct is further extended with a reserved :31 (or whatever) as
> opposed to going back and claiming gaps from the past. Maybe the
> former is OK.
I think "reserved" or not doesn't change anything, name it or not the
hole is there in any case.
Why extending firstbit:1;reserved:31 to e.g.
firstbit:1;secondbit:1;reserved:30 would be more compatible than the
implicit firstbit:1;secondbit:1 ?
Both should be allowed IMHO.
Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c
Posted by Eric Covener <co...@gmail.com>.
On Tue, Jun 26, 2018 at 8:37 AM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Mon, Jun 25, 2018 at 10:49 PM, Eric Covener <co...@gmail.com> wrote:
> >
> > Bill or Yann, do you remember the specific gotcha with setting aside
> > addl bits and re-using them later?
>
> I've never thought it was an issue (re compat) to add some bit(s) in a
> bitfield if there is a hole, wherever this field is in the struct.
> It doesn't change the size and there is no way for the user to have
> used the address of any bit in the first place (it can't break
> anything IMHO).
> Bill objected to this, I can't remember why (and he is in better
> position to explain it), status quo so far...
>
Just to be clear, I am talking about claiming the lost ones _before_
the struct is further extended with a reserved :31 (or whatever) as
opposed to going back and claiming gaps from the past. Maybe the
former is OK.
Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Jun 25, 2018 at 10:49 PM, Eric Covener <co...@gmail.com> wrote:
>
> Bill or Yann, do you remember the specific gotcha with setting aside
> addl bits and re-using them later?
I've never thought it was an issue (re compat) to add some bit(s) in a
bitfield if there is a hole, wherever this field is in the struct.
It doesn't change the size and there is no way for the user to have
used the address of any bit in the first place (it can't break
anything IMHO).
Bill objected to this, I can't remember why (and he is in better
position to explain it), status quo so far...
>
> 2.4.x currently has a single bit bitfield at the end of the server_rec.
Yes, I think there is much less interest in using bitfields if we
can't extend them.
Regards,
Yann.
Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c
Posted by Eric Covener <co...@gmail.com>.
On Wed, Apr 11, 2018 at 4:05 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Wed, Apr 11, 2018 at 9:14 PM, Eric Covener <co...@gmail.com> wrote:
> >> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
> >> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Wed Apr 11 19:11:52 2018
> >> @@ -459,6 +459,8 @@ typedef struct {
> >> char secret[PROXY_WORKER_MAX_SECRET_SIZE]; /* authentication secret (e.g. AJP13) */
> >> char upgrade[PROXY_WORKER_MAX_SCHEME_SIZE];/* upgrade protocol used by mod_proxy_wstunnel */
> >> char hostname_ex[PROXY_RFC1035_HOSTNAME_SIZE]; /* RFC1035 compliant version of the remote backend address */
> >> + apr_size_t response_field_size; /* Size of proxy response buffer in bytes. */
> >> + unsigned int response_field_size_set:1;
> >> } proxy_worker_shared;
> >
> >
> > If this is for trunk only, should I move the bit field up and call it
> > major? I don't plan to backport it.
>
> Maybe the backport is needed to resolve PR 62196 altogether?
>
> > Whether I move it or not, should I reserve the next range of bytes after it?
>
> Would be nice to rearrange the struct for trunk/2.next.
> As for bitfields I'm not sure it helps reserving names for unused bits
> since we can't extend them in stable versions anyway (I wish we could
> given that it doesn't change any size/address and bits themselves have
> no address, but IIRC from an earlier discussion with Bill it's not an
> option)
Bill or Yann, do you remember the specific gotcha with setting aside
addl bits and re-using them later?
2.4.x currently has a single bit bitfield at the end of the server_rec.
--
Eric Covener
covener@gmail.com
Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Apr 11, 2018 at 10:05 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Apr 11, 2018 at 9:14 PM, Eric Covener <co...@gmail.com> wrote:
>>> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
>>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Wed Apr 11 19:11:52 2018
>>> @@ -459,6 +459,8 @@ typedef struct {
>>> char secret[PROXY_WORKER_MAX_SECRET_SIZE]; /* authentication secret (e.g. AJP13) */
>>> char upgrade[PROXY_WORKER_MAX_SCHEME_SIZE];/* upgrade protocol used by mod_proxy_wstunnel */
>>> char hostname_ex[PROXY_RFC1035_HOSTNAME_SIZE]; /* RFC1035 compliant version of the remote backend address */
>>> + apr_size_t response_field_size; /* Size of proxy response buffer in bytes. */
>>> + unsigned int response_field_size_set:1;
>>> } proxy_worker_shared;
>>
>>
>> If this is for trunk only, should I move the bit field up and call it
>> major? I don't plan to backport it.
>
> Maybe the backport is needed to resolve PR 62196 altogether?
Scratch that, this commit doesn't fix the case where '\n' is within
the ENOSPC brigade but only comments on the issue.
Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Apr 11, 2018 at 9:14 PM, Eric Covener <co...@gmail.com> wrote:
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Wed Apr 11 19:11:52 2018
>> @@ -459,6 +459,8 @@ typedef struct {
>> char secret[PROXY_WORKER_MAX_SECRET_SIZE]; /* authentication secret (e.g. AJP13) */
>> char upgrade[PROXY_WORKER_MAX_SCHEME_SIZE];/* upgrade protocol used by mod_proxy_wstunnel */
>> char hostname_ex[PROXY_RFC1035_HOSTNAME_SIZE]; /* RFC1035 compliant version of the remote backend address */
>> + apr_size_t response_field_size; /* Size of proxy response buffer in bytes. */
>> + unsigned int response_field_size_set:1;
>> } proxy_worker_shared;
>
>
> If this is for trunk only, should I move the bit field up and call it
> major? I don't plan to backport it.
Maybe the backport is needed to resolve PR 62196 altogether?
> Whether I move it or not, should I reserve the next range of bytes after it?
Would be nice to rearrange the struct for trunk/2.next.
As for bitfields I'm not sure it helps reserving names for unused bits
since we can't extend them in stable versions anyway (I wish we could
given that it doesn't change any size/address and bits themselves have
no address, but IIRC from an earlier discussion with Bill it's not an
option). It doesn't prevent us from joining the existing ones for
2.next, though.
So +1 for me, including for an overall (optimized for size/holes)
moving of fields.
Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
If only for trunk then I would say Yes, lets optimize these struct fields.
> On Apr 11, 2018, at 3:14 PM, Eric Covener <co...@gmail.com> wrote:
>
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Wed Apr 11 19:11:52 2018
>> @@ -459,6 +459,8 @@ typedef struct {
>> char secret[PROXY_WORKER_MAX_SECRET_SIZE]; /* authentication secret (e.g. AJP13) */
>> char upgrade[PROXY_WORKER_MAX_SCHEME_SIZE];/* upgrade protocol used by mod_proxy_wstunnel */
>> char hostname_ex[PROXY_RFC1035_HOSTNAME_SIZE]; /* RFC1035 compliant version of the remote backend address */
>> + apr_size_t response_field_size; /* Size of proxy response buffer in bytes. */
>> + unsigned int response_field_size_set:1;
>> } proxy_worker_shared;
>
>
> If this is for trunk only, should I move the bit field up and call it
> major? I don't plan to backport it.
> Whether I move it or not, should I reserve the next range of bytes after it?