You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Isabel Jimenez <co...@isabeljimenez.com> on 2015/08/04 23:29:46 UTC
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37097/
-----------------------------------------------------------
(Updated Aug. 4, 2015, 9:29 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos
Description
-------
Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip'
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
Diff: https://reviews.apache.org/r/37097/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37097/#review94161
-----------------------------------------------------------
3rdparty/libprocess/src/http.cpp
<https://reviews.apache.org/r/37097/#comment148696>
What is the rationale behind moving these comments to the header file ? I wasn't able to make it out from r36402, Pardon my ignorance.
Having these rules in the cpp file is much more intuitive since it allows you to easily understand the code as each rule is followed by its implementation.
Including the rules in the header file is essentially duplicating the RFC. Why do we want to do it ?
3rdparty/libprocess/src/tests/encoder_tests.cpp (line 68)
<https://reviews.apache.org/r/37097/#comment148695>
Can we kill the extra [0] index element we just introduced ?
- Anand Mazumdar
On Aug. 4, 2015, 10:43 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37097/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 10:43 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip'
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
>
> Diff: https://reviews.apache.org/r/37097/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37097/#review94386
-----------------------------------------------------------
3rdparty/libprocess/src/http.cpp (lines 133 - 138)
<https://reviews.apache.org/r/37097/#comment148962>
Can you leave the RFC number here? I find it is a good practice even if it is repeatedly used.
3rdparty/libprocess/src/http.cpp (lines 134 - 138)
<https://reviews.apache.org/r/37097/#comment148963>
This paragraph doesn't seem to come from the RFC. Can you move it before the headline or after the contents from the rfc?
3rdparty/libprocess/src/http.cpp (line 159)
<https://reviews.apache.org/r/37097/#comment148964>
I don't think we use `cout` in libprocess but `VLOG` (check `decode` in this file).
- Alexander Rojas
On Aug. 5, 2015, 9 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37097/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2015, 9 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip'
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
>
> Diff: https://reviews.apache.org/r/37097/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37097/#review94513
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/http.cpp (line 133)
<https://reviews.apache.org/r/37097/#comment149139>
s/2606/2616/
- Alexander Rojas
On Aug. 6, 2015, 7:35 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37097/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2015, 7:35 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip'
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
>
> Diff: https://reviews.apache.org/r/37097/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37097/#review94958
-----------------------------------------------------------
Ship it!
Couple of comments below, but I'll make the adjustments and get this committed for you shortly! Thanks for splitting this out and updating the test!
3rdparty/libprocess/include/process/http.hpp (line 120)
<https://reviews.apache.org/r/37097/#comment149686>
We're still wrapping comments at 70 for now, but that might change soon :)
3rdparty/libprocess/include/process/http.hpp (line 122)
<https://reviews.apache.org/r/37097/#comment149684>
Which RFC? :)
3rdparty/libprocess/include/process/http.hpp (lines 124 - 125)
<https://reviews.apache.org/r/37097/#comment149683>
This bit seems to just be re-iterating the summary above?
3rdparty/libprocess/src/http.cpp (lines 134 - 135)
<https://reviews.apache.org/r/37097/#comment149692>
Reading through the RFC again, it's quite a bit more subtle than this:
```
4. The "identity" content-coding is always acceptable, unless
specifically refused because the Accept-Encoding field includes
"identity;q=0", or because the field includes "*;q=0" and does
not explicitly include the "identity" content-coding. If the
Accept-Encoding field-value is empty, then only the "identity"
encoding is acceptable.
If no Accept-Encoding field is present in a request, the server MAY assume that the client will accept any content coding. In this case, if "identity" is one of the available content-codings, then the server SHOULD use the "identity" content-coding, unless it has additional information that a different content-coding is meaningful to the client.
Note: If the request does not include an Accept-Encoding field,
and if the "identity" content-coding is unavailable, then
content-codings commonly understood by HTTP/1.0 clients (i.e.,
"gzip" and "compress") are preferred; some older clients
improperly display messages sent with other content-codings. The
server might also make this decision based on information about
the particular user-agent or client.
Note: Most HTTP/1.0 applications do not recognize or obey qvalues
associated with content-codings. This means that qvalues will not
work and are not permitted with x-gzip or x-compress.
```
So it appears that we're doing the right thing here by returning false and using the identity encoding, but we don't correctly deal with explicitly rejected identity encoding for now.. I'll remove this and the TODO since it's a bit misleading
3rdparty/libprocess/src/http.cpp (lines 145 - 147)
<https://reviews.apache.org/r/37097/#comment149688>
Shouldn't this be up where we're returning false..?
3rdparty/libprocess/src/http.cpp (line 159)
<https://reviews.apache.org/r/37097/#comment149701>
Hm.. it's a bit implicit that tokens.empty is guaranteed to be false here because 'encoding' itself is non-empty due to it coming from tokenize.
- Ben Mahler
On Aug. 7, 2015, 6:52 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37097/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 6:52 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip'
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp b8d9300
> 3rdparty/libprocess/src/http.cpp 4dcbd74
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
>
> Diff: https://reviews.apache.org/r/37097/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37097/
-----------------------------------------------------------
(Updated Aug. 7, 2015, 6:52 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos
Description
-------
Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip'
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
Diff: https://reviews.apache.org/r/37097/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37097/
-----------------------------------------------------------
(Updated Aug. 6, 2015, 5:35 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos
Description
-------
Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip'
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
Diff: https://reviews.apache.org/r/37097/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37097/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 7 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos
Description
-------
Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip'
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
Diff: https://reviews.apache.org/r/37097/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37097/
-----------------------------------------------------------
(Updated Aug. 4, 2015, 10:43 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos
Description
-------
Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip'
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp b8d9300
3rdparty/libprocess/src/http.cpp 4dcbd74
3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137
Diff: https://reviews.apache.org/r/37097/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez